fix: check HTTP status before parsing JSON in multipart upload#146
Merged
Merged
Conversation
- Reorder handle_response_error() before json.loads() in _upload_part, _initiate_multipart_upload, and _complete_multipart_upload so that server errors (empty body, 502, etc.) raise a clear ResponseError instead of a cryptic JSONDecodeError. - Reduce MAX_PARALLEL_UPLOADS from 20 to 3 (matching the TypeScript SDK) to avoid overwhelming the sandbox API. - Replace print() with logger.warning() in _abort_multipart_upload. Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Contributor
Author
🤖 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:
|
Contributor
There was a problem hiding this comment.
LGTM
The fix is correct. handle_response_error() raises on error status codes, preventing json.loads() from running on empty/error bodies — the finally block still closes the response regardless. The data → result rename in _upload_part also eliminates a variable shadowing issue with the data: bytes parameter. No correctness, security, or data-loss issues.
Tag @mendral-app with feedback or questions. View session
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes
JSONDecodeError: Expecting value: line 1 column 1 (char 0)when uploading large files viawrite_binary()/write()with multipart upload.Root cause: In the async multipart upload methods (
_upload_part,_initiate_multipart_upload,_complete_multipart_upload),json.loads(await response.aread())was called beforehandle_response_error(response). When the server returned an error with an empty body (e.g., 502 from the gateway under load),json.loadsblew up with a crypticJSONDecodeErrorinstead of a clearResponseErrorwith status code info.Changes:
handle_response_error()beforejson.loads()in all three async multipart methods so HTTP errors surface asResponseErrorwith status code/body info.MAX_PARALLEL_UPLOADSfrom 20 → 3 (matching the TypeScript SDK) to reduce the likelihood of overwhelming the sandbox API.print()withlogger.warning()in_abort_multipart_upload(per repo conventions: noprint()in library code).The sync version already had the correct ordering (
handle_response_errorbeforeresponse.json()); only the parallel upload limit was updated there.Review & Testing Checklist for Human
sandbox.fs.write_binary()to confirm multipart upload succeeds end-to-endResponseErrorwith status info instead ofJSONDecodeErrorNotes
mkdir,writefor small files, etc.) have the samejson.loads-before-error-check pattern but are much less likely to hit empty-body errors since they're simpler single-request operations. Could be a follow-up cleanup._upload_partalready had the correct ordering; only theMAX_PARALLEL_UPLOADSconstant was changed there.Link to Devin session: https://app.devin.ai/sessions/dd4c5d3cf8d74415a761f7b614cd5184
Requested by: @Joffref
Note
Fixes a bug where HTTP error responses with empty bodies (e.g., 502s) during multipart upload caused a cryptic
JSONDecodeErrorinstead of a properResponseError. The fix reordershandle_response_error()beforejson.loads()in the three async multipart methods, reducesMAX_PARALLEL_UPLOADSfrom 20 to 3 to match the TypeScript SDK, and replaces aprint()withlogger.warning().Written by Mendral for commit 9c53176.