fix: catch OSError in websocket send to prevent server crash send_soc…

[Chaudhary-Harshit]

fix: catch OSError in websocket send to prevent server crash

Summary

Fixes #1442. A websocket client that becomes unreachable mid-generation
could crash the entire ComfyUI server with an uncaught OSError.

When it happens

While a generation is running, the server pushes progress updates to
connected clients over the websocket. If a client disappears abruptly —
laptop sleeps, drops off WiFi/VPN, remote route breaks — the OS raises
OSError: [Errno 113] No route to host (EHOSTUNREACH) on the next send.

Root cause

send_socket_catch_exception (server.py) only caught ConnectionError
and its subclasses:

except (aiohttp.ClientError, aiohttp.ClientPayloadError,
        ConnectionResetError, BrokenPipeError, ConnectionError) as err:

EHOSTUNREACH is raised as a plain OSError, which is the parent of
ConnectionError and a sibling of the connection errors listed  and not a
subclass. It therefore escaped the handler, propagated up through
publish_loop, and took the whole server down over a single dead client.

Fix

Catch OSError directly. As the parent of the connection errors already
handled, it subsumes them and additionally covers EHOSTUNREACH:

except (aiohttp.ClientError, aiohttp.ClientPayloadError, OSError) as err:

Testing

Added tests-unit/server_test/test_send_socket_catch_exception.py:

- test_swallows_no_route_to_host — reproduces #1442 (OSError(EHOSTUNREACH));
fails before this change, passes after.
- test_swallows_connection_reset — regression guard for the existing
ConnectionResetError behaviour.
- test_successful_send_delivers_message — confirms normal messages are
still delivered (errors are swallowed, valid sends are not).

$ python -m pytest tests-unit/server_test/test_send_socket_catch_exception.py
3 passed

$ python -m pytest tests-unit
900 passed, 10 skipped

[Chaudhary-Harshit]

@comfyanonymous Please review the PR

[coderabbitai]

📝 Walkthrough

Walkthrough

send_socket_catch_exception in server.py has its exception handler narrowed from several explicit connection-related types (ConnectionResetError, BrokenPipeError, ConnectionError, etc.) to aiohttp.ClientError, aiohttp.ClientPayloadError, and the base OSError. A new async pytest module test_send_socket_catch_exception.py is added with three test cases: one asserts that OSError(113, "No route to host") is swallowed and logged, one confirms ConnectionResetError is still caught, and one verifies that a successful send delivers the payload without logging.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 28.57% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The PR title accurately captures the main change: catching OSError in websocket send to prevent server crashes, addressing the core issue.
Description check	✅ Passed	The description thoroughly explains the issue, root cause, fix, and testing approach, directly relating to the changeset.
Linked Issues check	✅ Passed	The PR directly addresses issue #1442 by catching OSError (including EHOSTUNREACH) that was previously unhandled, preventing server crashes from disconnected clients.
Out of Scope Changes check	✅ Passed	All changes are directly scoped to fixing the identified issue: modifying exception handling in send_socket_catch_exception and adding comprehensive unit tests.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

🧹 Nitpick comments (1)

tests-unit/server_test/test_send_socket_catch_exception.py (1)

30-38: 💤 Low value

Consider verifying the "nothing is logged" claim in the docstring.

The docstring states that nothing is logged when the send succeeds, but this assertion is not included in the test. While the test correctly verifies the message is delivered, adding a caplog check would ensure the documented behavior is validated.

📝 Suggested enhancement

-    async def test_successful_send_delivers_message(self):
+    async def test_successful_send_delivers_message(self, caplog):
         """When the send succeeds the message is passed through and nothing is logged."""
         received = []
 
         async def ok_send(message):
             received.append(message)
 
         await send_socket_catch_exception(ok_send, b"hello")
         assert received == [b"hello"]
+        assert "send error" not in caplog.text

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests-unit/server_test/test_send_socket_catch_exception.py` around lines 30 -
38, The test method test_successful_send_delivers_message has a docstring
claiming that nothing is logged when the send succeeds, but the test only
verifies the message delivery without actually asserting that no logs were
captured. Add a caplog fixture parameter to the test method and include an
assertion to verify that the caplog is empty or contains no log records, thus
validating the documented behavior that nothing should be logged during
successful message delivery.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@tests-unit/server_test/test_send_socket_catch_exception.py`:
- Around line 30-38: The test method test_successful_send_delivers_message has a
docstring claiming that nothing is logged when the send succeeds, but the test
only verifies the message delivery without actually asserting that no logs were
captured. Add a caplog fixture parameter to the test method and include an
assertion to verify that the caplog is empty or contains no log records, thus
validating the documented behavior that nothing should be logged during
successful message delivery.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: a7b0397b-43d9-4271-800e-1b587682bc74

📥 Commits

Reviewing files that changed from the base of the PR and between e1b9366 and a4342e9.

📒 Files selected for processing (2)

• server.py
• tests-unit/server_test/test_send_socket_catch_exception.py