fix(handlers): prevent daemon crash on non-serializable SWIG objects (B59)#163
fix(handlers): prevent daemon crash on non-serializable SWIG objects (B59)#163BANANASJIM merged 2 commits intomasterfrom
Conversation
…(B59) _enum_name now returns str() for non-primitive objects without .name, and run_server catches TypeError on json.dumps to return a JSON-RPC error instead of crashing the daemon process.
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe changes add robust serialization error handling for JSON-RPC responses. When Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
PR Compliance Guide 🔍Below is a summary of compliance checks for this PR:
Compliance status legend🟢 - Fully Compliant🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label |
||||||||||||||||||||||||||
PR Code Suggestions ✨Explore these optional code suggestions:
|
||||||||||||
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/unit/test_daemon_output_quality.py (1)
81-90: Strengthen the SWIG fallback assertion to validate exact behaviorLine 89 only checks type; asserting exact
str(obj)output makes this test more regression-resistant.Proposed test tightening
obj = SwigObj() result = _enum_name(obj) assert isinstance(result, str) + assert result == str(obj)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/test_daemon_output_quality.py` around lines 81 - 90, The test test_swig_fallback_returns_str should assert the exact fallback string value rather than just the type: replace or augment the existing isinstance assertion with an equality check that result == str(obj) to ensure _enum_name(obj) returns the same string as str(obj) for objects without a .name (referencing the _enum_name function and the SwigObj instance in the test).tests/unit/test_daemon_server_unit.py (1)
541-569: Avoid duplicating fallback logic inside the testThis block re-implements the server’s serialization-guard branch. Prefer asserting behavior through shared helper logic (or an integration-style run_server path) so test and production cannot silently diverge.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/test_daemon_server_unit.py` around lines 541 - 569, The test test_json_dumps_guard_produces_valid_error is re-implementing the server's serialization-fallback logic; instead, call the actual serialization guard used by run_server (or the shared helper the server uses to produce the error payload) to get the payload and assert on it. Replace the try/except/json.dumps block with an invocation of the server's serialization helper (the same function run_server delegates to when catching TypeError) and assert that the returned JSON contains id == 42, error.code == -32603 and that the error.message contains "serialization error".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/rdc/daemon_server.py`:
- Around line 438-446: The except block currently only catches TypeError from
json.dumps, so other serialization failures (ValueError, RecursionError, or any
other Exception) can escape; update the exception handling around the json.dumps
call used to build err_resp (the block that sets err_resp, payload, and
binary_path) to catch ValueError and RecursionError (or catch Exception broadly)
and log the exception (_log.warning/_.error) and produce the same JSON-RPC error
response using response.get("id"), ensuring payload is set to
json.dumps(err_resp) + "\n" and binary_path is set to None; keep the same error
code/message structure but include the exception details in the message.
---
Nitpick comments:
In `@tests/unit/test_daemon_output_quality.py`:
- Around line 81-90: The test test_swig_fallback_returns_str should assert the
exact fallback string value rather than just the type: replace or augment the
existing isinstance assertion with an equality check that result == str(obj) to
ensure _enum_name(obj) returns the same string as str(obj) for objects without a
.name (referencing the _enum_name function and the SwigObj instance in the
test).
In `@tests/unit/test_daemon_server_unit.py`:
- Around line 541-569: The test test_json_dumps_guard_produces_valid_error is
re-implementing the server's serialization-fallback logic; instead, call the
actual serialization guard used by run_server (or the shared helper the server
uses to produce the error payload) to get the payload and assert on it. Replace
the try/except/json.dumps block with an invocation of the server's serialization
helper (the same function run_server delegates to when catching TypeError) and
assert that the returned JSON contains id == 42, error.code == -32603 and that
the error.message contains "serialization error".
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/rdc/daemon_server.pysrc/rdc/handlers/_helpers.pytests/unit/test_daemon_output_quality.pytests/unit/test_daemon_server_unit.py
Greptile SummaryThis PR fixes a daemon crash (B59) caused by non-serializable SWIG objects reaching
Confidence Score: 4/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["Handler returns result"] --> B{"_enum_name called?"}
B -->|"has .name"| C["return v.name"]
B -->|"str / int / float / None"| D["pass through"]
B -->|"other type (SWIG obj etc.)"| E["log debug warning\nreturn str(v)"]
C --> F["response dict assembled"]
D --> F
E --> F
F --> G["json.dumps(response)"]
G -->|"success"| H["payload = serialised JSON"]
G -->|"TypeError / ValueError / RecursionError"| I["log warning\nbuild -32603 error response\nbinary_path = None"]
I --> J["payload = json.dumps(err_resp)"]
H --> K["conn.sendall(payload)"]
J --> K
K -->|"binary_path set"| L["send binary file"]
K -->|"no binary"| M["continue loop"]
L --> M
Last reviewed commit: 7c2ad5d |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| try: | ||
| conn.sendall((json.dumps(response) + "\n").encode("utf-8")) | ||
| payload = json.dumps(response) + "\n" | ||
| except TypeError as exc: | ||
| _log.warning("serialization error: %s", exc) | ||
| err_resp: dict[str, Any] = { | ||
| "jsonrpc": "2.0", | ||
| "id": response.get("id"), | ||
| "error": {"code": -32603, "message": f"serialization error: {exc}"}, | ||
| } | ||
| payload = json.dumps(err_resp) + "\n" | ||
| binary_path = None |
There was a problem hiding this comment.
ValueError not caught — daemon still crashes on circular references
json.dumps can also raise ValueError (e.g. for circular-reference structures or NaN/Infinity when allow_nan=False). The guard only catches TypeError, so a response containing a circular dict would still propagate an unhandled exception and terminate the connection handler — the exact crash the fix is trying to prevent.
| try: | |
| conn.sendall((json.dumps(response) + "\n").encode("utf-8")) | |
| payload = json.dumps(response) + "\n" | |
| except TypeError as exc: | |
| _log.warning("serialization error: %s", exc) | |
| err_resp: dict[str, Any] = { | |
| "jsonrpc": "2.0", | |
| "id": response.get("id"), | |
| "error": {"code": -32603, "message": f"serialization error: {exc}"}, | |
| } | |
| payload = json.dumps(err_resp) + "\n" | |
| binary_path = None | |
| payload = json.dumps(response) + "\n" | |
| except (TypeError, ValueError) as exc: |
| if hasattr(v, "name"): | ||
| return v.name | ||
| if isinstance(v, (str, int, float)) or v is None: | ||
| return v | ||
| return str(v) |
There was a problem hiding this comment.
Silent str() conversion may hide unexpected object types
The original function returned v unchanged for any non-.name type, which meant the JSON serializer would surface the problem explicitly at the serialization boundary. The new fallback silently coerces any unrecognized object to a string, making it harder to detect cases where a handler is returning an unexpected type (e.g. a dict or list that was previously passed through and JSON-serialized correctly would now be stringified).
Consider logging a warning before the str() coercion so that unexpected types are surfaced during development:
import logging as _logging
_log = _logging.getLogger(__name__)
_log.debug("_enum_name: unexpected type %s, coercing to str", type(v).__name__)
return str(v)All current call-sites use SWIG enums or empty-string defaults, so this is low risk in practice, but the silent coercion could mask future bugs.
| try: | ||
| json.dumps(response) | ||
| payload_ok = True | ||
| except TypeError as exc: | ||
| err_resp: dict[str, Any] = { | ||
| "jsonrpc": "2.0", | ||
| "id": response.get("id"), | ||
| "error": {"code": -32603, "message": f"serialization error: {exc}"}, | ||
| } | ||
| payload = json.dumps(err_resp) | ||
| payload_ok = False | ||
|
|
||
| assert not payload_ok | ||
| parsed = json.loads(payload) | ||
| assert parsed["id"] == 42 | ||
| assert parsed["error"]["code"] == -32603 | ||
| assert "serialization error" in parsed["error"]["message"] |
There was a problem hiding this comment.
payload may be unbound if the try block unexpectedly succeeds
payload is only assigned inside the except TypeError branch. If json.dumps(response) were ever to succeed (e.g. if Python's JSON encoder changes, or if Unserializable gains a __json__ method in a future version), payload_ok would be True, the assert not payload_ok would catch that — but the subsequent json.loads(payload) would raise UnboundLocalError before the assertion is even evaluated, producing a misleading test failure.
Initialise the variable before the try/except to make the test resilient:
payload = ""
try:
json.dumps(response)
payload_ok = True
except TypeError as exc:
...- Extend JSON serialization guard to also catch ValueError and RecursionError - Add debug log in _enum_name before str() coercion of unexpected types - Initialize payload variable before try/except to prevent UnboundLocalError
| try: | ||
| json.dumps(response) | ||
| payload_ok = True | ||
| except TypeError as exc: | ||
| err_resp: dict[str, Any] = { | ||
| "jsonrpc": "2.0", | ||
| "id": response.get("id"), | ||
| "error": {"code": -32603, "message": f"serialization error: {exc}"}, | ||
| } | ||
| payload = json.dumps(err_resp) | ||
| payload_ok = False |
There was a problem hiding this comment.
payload_ok unbound for non-TypeError exceptions
payload_ok is only assigned inside the try or except TypeError branches. If json.dumps(response) were to raise ValueError or RecursionError (both of which are caught in the production guard), neither branch would execute, leaving payload_ok unbound. The subsequent assert not payload_ok would then raise a misleading NameError rather than a clean test failure.
Note: the production code in run_server now catches (TypeError, ValueError, RecursionError), so the test surface is slightly narrower than production. Initialising payload_ok before the block makes the test resilient to that mismatch:
| try: | |
| json.dumps(response) | |
| payload_ok = True | |
| except TypeError as exc: | |
| err_resp: dict[str, Any] = { | |
| "jsonrpc": "2.0", | |
| "id": response.get("id"), | |
| "error": {"code": -32603, "message": f"serialization error: {exc}"}, | |
| } | |
| payload = json.dumps(err_resp) | |
| payload_ok = False | |
| payload = "" | |
| payload_ok = True | |
| try: | |
| json.dumps(response) | |
| except TypeError as exc: | |
| err_resp: dict[str, Any] = { | |
| "jsonrpc": "2.0", | |
| "id": response.get("id"), | |
| "error": {"code": -32603, "message": f"serialization error: {exc}"}, | |
| } | |
| payload = json.dumps(err_resp) | |
| payload_ok = False |
1 participant
User description
Summary
_enum_name()now appliesstr()to non-primitive objects without.name, preventing raw SWIG objects from reaching JSON serializationjson.dumps()now catchesTypeErrorand returns a JSON-RPC-32603error instead of crashing the processcat /draws/<eid>/descriptorskilled the daemon, causing all subsequent tests to get "no active session"Test plan
_enum_nameSWIG fallback returns strpixi run lint && pixi run test— 2449 passed, 93.81% coveragePR Type
Bug fix
Description
Prevent daemon crash when handlers return non-serializable SWIG objects
_enum_name()now converts non-primitive objects to strings safelyrun_server()catchesTypeErrorduring JSON serialization and returns JSON-RPC errorFixes cascade e2e test failures from daemon process termination
Diagram Walkthrough
File Walkthrough
daemon_server.py
Add TypeError guard to json.dumps serializationsrc/rdc/daemon_server.py
json.dumps(response)in try-except to catchTypeError-32603binary_pathwhen serialization fails to prevent filetransmission
_helpers.py
Convert non-serializable objects to strings safelysrc/rdc/handlers/_helpers.py
_enum_name()to handle non-primitive objects without.nameattribute
unchanged
str()test_daemon_output_quality.py
Add tests for SWIG object string conversiontests/unit/test_daemon_output_quality.py
test_swig_fallback_returns_str()to verify SWIG objects convertto strings
test_float_passthrough()to ensure float primitives pass throughunchanged
test_daemon_server_unit.py
Add serialization resilience tests for daemontests/unit/test_daemon_server_unit.py
TestSerializationResilienceclass with two test methodstest_non_serializable_result_returns_error()verifies handler canreturn unserializable objects
test_json_dumps_guard_produces_valid_error()validates the errorresponse structure
-32603and proper error messageformatting
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests