fix: handle 504/non-JSON error responses properly instead of crashing with JSONDecodeError#111
Conversation
In the async Python SDK, json.loads() was called before handle_response_error(), so non-JSON error responses (like 504 Gateway Timeout HTML pages) would crash with JSONDecodeError instead of raising the proper ResponseError. Fixed by reordering: read bytes first, check HTTP status via handle_response_error(), then parse JSON. This ensures that 504 and other HTTP errors raise ResponseError with status code info rather than confusing JSON parse errors. Affected methods: - process.get(), list(), stop(), kill(), logs() (async) - process._exec_with_streaming() (async + sync) - improved error message - filesystem mkdir(), write(), write_tree(), read(), rm(), ls(), find(), grep(), and multipart upload helpers (async) Co-Authored-By: cdrappier <cdrappier@blaxel.ai>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
![mendral-app[bot]](https://avatars.githubusercontent.com/in/1975249?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
Co-Authored-By: cdrappier <cdrappier@blaxel.ai>
There was a problem hiding this comment.
🤖 Code Review
Assessment ⚠️
The core fix (committed in 301ed3b) was already reviewed and approved. The only new change in this update is the addition of reproducer_504.py.
The reproducer script is well-structured and covers all the fixed methods. One issue: the assert_raises_clean_error helper's fallback except Exception branch (lines 135–141) counts any unexpected exception type as a pass (with a WARN label). This means a regression introducing a new unexpected exception type would silently pass the test suite. Consider this if the script is intended to be used as a regression gate.
Note
Tag @mendral-app with feedback or questions. View session
| except Exception as e: | ||
| if "504" in str(e) or "status" in str(e).lower(): | ||
| print(f" PASS {method_name}: raised Exception with status info: {e}") | ||
| passed += 1 | ||
| else: | ||
| print(f" WARN {method_name}: raised {type(e).__name__}: {e}") | ||
| passed += 1 # Still better than JSONDecodeError/AttributeError |
There was a problem hiding this comment.
🐛 bug (low)
The fallback except Exception branch counts any unexpected exception as a pass (incrementing passed). If a new bug introduced a different exception type (e.g. ConnectionError, TimeoutError), the test would print WARN but still count as passing and not fail the script. If this script is used as a regression gate, unexpected exceptions should be counted as failures.
📝 Suggested change
| except Exception as e: | |
| if "504" in str(e) or "status" in str(e).lower(): | |
| print(f" PASS {method_name}: raised Exception with status info: {e}") | |
| passed += 1 | |
| else: | |
| print(f" WARN {method_name}: raised {type(e).__name__}: {e}") | |
| passed += 1 # Still better than JSONDecodeError/AttributeError | |
| except Exception as e: | |
| if "504" in str(e) or "status" in str(e).lower(): | |
| print(f" PASS {method_name}: raised Exception with status info: {e}") | |
| passed += 1 | |
| else: | |
| print(f" FAIL {method_name}: raised unexpected {type(e).__name__}: {e}") | |
| failed += 1 |
1 participant
fix: check HTTP status before JSON parsing to handle 504/error responses
Summary
When the sandbox proxy returns a non-JSON error response (e.g. 504 Gateway Timeout with an HTML body), the async Python SDK was crashing with
json.JSONDecodeError("Expecting value: line 1 column 1") instead of raising a properResponseErrorwith the HTTP status code.Root cause:
json.loads(await response.aread())was called beforeself.handle_response_error(response)in every async method that uses raw httpx. This meant the JSON parse always ran first and blew up on HTML/non-JSON bodies, preventing the HTTP status check from ever executing.Fix: Reorder to: read bytes → check HTTP status via
handle_response_error()→ parse JSON. This matches the pattern already used correctly in the sync SDK.Affected async methods:
process.get(),list(),stop(),kill(),logs()filesystem.mkdir(),write(),write_tree(),read(),rm(),ls(),find(),grep(), and multipart upload helpersAlso improved
_exec_with_streaming()error messages (async + sync) to include the HTTP status code.Review & Testing Checklist for Human
handle_response_error(response)must come afterawait response.aread()(httpx requires reading before checking) but beforejson.loads()sync/process.pyget/list/stop/kill/logs) were already correct and intentionally left unchanged_upload_part, variable was renamed fromdatatoresult_datato avoid shadowing the function parameter — verify this doesn't break callersprocess.exec()orprocess.get()) to confirmResponseErroris now raised instead ofJSONDecodeErrorNotes
handle_response_errorbeforeresponse.json()) so only the_exec_with_streamingerror message was improved thereNote
This PR fixes a bug where async SDK methods called
json.loads()beforehandle_response_error(), causingJSONDecodeErroron non-JSON error responses (e.g. 504 HTML pages). The fix reorders to: read bytes → check HTTP status → parse JSON, across all affected async methods infilesystem.pyandprocess.py. The incremental commit adds areproducer_504.pyvalidation script.Written by Mendral for commit e39ca55.