fix: read error response body in _get_file_request to enable 401 retry - #1084
Conversation
There was a problem hiding this comment.
Code Review
This pull request improves error handling and retry logic for GCS requests in gcsfs. Specifically, it updates _get_file_request to validate responses immediately upon receiving a status code of 400 or higher by reading the response content. In validate_response, it enhances robustness when parsing JSON error payloads, decodes content using UTF-8 with replacement fallback, and ensures that 401 authentication errors containing 'invalid' are raised as retriable HttpErrors rather than ValueErrors. Corresponding unit tests have been added to verify these edge cases and retry behaviors. There are no review comments to address.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1084 +/- ##
==========================================
+ Coverage 90.25% 90.39% +0.14%
==========================================
Files 16 16
Lines 3755 3811 +56
==========================================
+ Hits 3389 3445 +56
Misses 366 366 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@Yonghui-Lee @zhixiangli |
|
Hi @rafaellott. A new release is scheduled in a week. I hope that doesn't block you. |
|
Hi @Yonghui-Lee , That sounds great to me. Thank you! |
Fixes #1082
In
_get_file_request,validate_response(r.status, None, rpath)was called before reading the response body. When GCS returned an intermittentHTTP 401with{"error": {"code": 401, "message": "Invalid Credentials"}},validate_responsereceivedcontent=Noneand raisedHttpError({"code": 401, "message": ""}). Becauseis_retriableonly retries401when"Invalid Credentials"is inexception.message, file downloads failed immediately without retrying.Changes
gcsfs/core.py(_get_file_request):r.status >= 400so error details reachvalidate_responseandretry_request, while preserving chunked streaming to disk for2xxresponses.callback = callback or NoOpCallback()to avoidAttributeErrorwhen_get_file_requestis invoked without a callback.validate_response(r.status, data, rpath)call after the read loop.gcsfs/retry.py(validate_response):status == 401from theelif status != 401 and "invalid" in str(msg):ValueErrorcheck so 401 responses containing lowercase"invalid"(e.g.,"Invalid Credentials: invalid_token") surface asHttpErrorand remain retriable.HttpError({"code": status, **error})preserves the HTTPstatuscode if"code"is missing from the parsed JSON"error"dictionary.errors="replace") and valid JSON payloads that do not match the{"error": {"message": ...}}structure.gcsfs/tests/test_retry.py:test_get_file_request_retries_401_invalid_credentialsandtest_validate_response_401_and_json_edge_cases.