Skip to content

fix(websocket): time out stalled sends and reset failed connections - #240

Merged
loks0n merged 4 commits into
mainfrom
codex/websocket-send-timeout
Sep 9, 2026
Merged

fix(websocket): time out stalled sends and reset failed connections#240
loks0n merged 4 commits into
mainfrom
codex/websocket-send-timeout

Conversation

@loks0n

@loks0n loks0n commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Swoole can leave push() coroutines suspended after a slow WebSocket client disconnects, retaining their messages indefinitely (swoole/swoole-src#6196). The adapter also ignores failed pushes, so a connected client can miss events without being disconnected.

Set a five-second default send_timeout, configured once with the Swoole adapter’s sendTimeout constructor parameter (new Swoole(sendTimeout: 2.0)). The parameter accepts finite positive seconds; there is no timeout setter. Keep yielding enabled so brief stalls can recover. If a push fails and the connection still exists, reset it immediately to discard queued output; a graceful close can run onClose while retaining the undrained socket. Clients must reconnect and refresh application state after a failed send.

This is the timeout-based alternative to #225, following the Swoole maintainer's workaround. It leaves the existing output buffer size unchanged. The timeout applies to each wait, and a retry can start another wait; it is not an absolute send deadline or a cap on pending bytes. Large bursts still require application backpressure or fewer updates. Appwrite will need to consume the released library to receive this behavior.

Validation

  • bin/monorepo check websocket: Pint, PHPStan, and Rector pass.
  • bin/monorepo test websocket: 9 unit tests and 5 end-to-end tests pass on macOS/PHP 8.5.9/Swoole 6.2.2, including Workerman coverage.
  • The 4 Swoole end-to-end tests also pass on Linux amd64/PHP 8.5.9/Swoole 6.2.2 in a local container. The container lacks pcntl, so its Workerman fixture was not run.
  • New regressions verify client-visible transport failure for a stalled peer, payload-memory recovery after disconnection with 8 MiB of runtime headroom, continued service to existing and newly connected clients, and all 300 frames arriving in order when a brief stall recovers. They do not assert native connection or coroutine counts.
  • Mutation checks fail at the intended assertions when the timeout is made unlimited or reset-close is replaced with graceful close. Restoring the implementation passes.
  • bin/monorepo validate, Vale, and git diff --check pass.

@greptile-apps

greptile-apps Bot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

RetriggerView in GreptileConfidence Score: 5/5

The PR appears safe to merge, with no outstanding correctness, security, or repository-rule finding.

Summary

  • Documents timeout semantics and client recovery expectations.
  • Validates the constructor timeout and applies it through Swoole configuration.
  • Adds behavioral coverage for stalled peers, peer disconnection, healthy-client continuity, and recovery from brief stalls.
  • Revises the recovery test so expected messages are supplied by the test rather than duplicated from fixture generation logic.

Comment thread packages/websocket/src/WebSocket/Adapter/Swoole.php Outdated
Comment thread packages/websocket/tests/E2E/AdapterTest.php Outdated
@loks0n

loks0n commented Sep 9, 2026

Copy link
Copy Markdown
Contributor Author

@greptileai Please re-review the current commit with the source-backed explanation in #240 (comment) and the test changes described in #240 (comment). The previous run started at 12:05:42 UTC, before those replies were posted.

The remaining descriptor-reuse warning assumes the PHP API receives a raw OS descriptor. In Swoole it receives a session ID, captured by value in this closure. exist() calls get_connection_verify(), which rejects mismatched session IDs even when a socket fd or session-table slot is reused. The linked Swoole 6.2.2 allocation and verification source establishes this independently of the variable rename. Please reassess that finding against those implementations and report any remaining actionable issue.

Comment thread packages/websocket/tests/E2E/AdapterTest.php Outdated
@loks0n
loks0n merged commit c327cfa into main Sep 9, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant