Repository navigation
Keep negative-size reads within upload chunk boundaries - #423
Conversation
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #423 +/- ##
===========================================
+ Coverage 80.28% 81.64% +1.36%
===========================================
Files 16 16
Lines 3013 3188 +175
===========================================
+ Hits 2419 2603 +184
+ Misses 594 585 -9 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
alexgromero
left a comment
alexgromero
left a comment
There was a problem hiding this comment.
Thanks for the report and the fix! This looks good overall and fixes the reported issue. I left one minor suggestion for a related legacy edge case, if you're open to including it in this PR.
|
|
||
| def read(self, amount=None): | ||
| if amount is None: | ||
| if amount is None or amount < 0: |
There was a problem hiding this comment.
After seeking beyond the chunk boundary, read() or read(-1) can still read from the underlying file because the remaining size is negative. Would you mind calculating amount_left = max(self._size - self._amount_read, 0) here matching utils.ReadFileChunk and adding a regression test for that case?
There was a problem hiding this comment.
Thanks, good catch. I've added the same clamp to the legacy reader, matching utils.ReadFileChunk, plus a regression test that seeks past the chunk boundary but stays inside the underlying file. It covers read-all, negative, zero and positive sizes. It checks that nothing is read, the position doesn't move and the callback reports 0 bytes, and that seeking back still reads the chunk normally. The new cases fail on the previous head and pass now. The unit and functional suites pass in threaded and serial modes on Linux with Python 3.12 and 3.14.
alexgromero
left a comment
alexgromero
left a comment
There was a problem hiding this comment.
Thanks for addressing the feedback and adding the additional regression coverage! This looks good to me 
* release-0.19.3: Bumping version to 0.19.3 Keep negative-size reads within upload chunk boundaries (#423) Add GitHub issue templates Bump https://github.com/astral-sh/ruff-pre-commit (#421) Bump https://github.com/astral-sh/ruff-pre-commit (#414) Bump the github-actions group across 1 directory with 6 updates (#420) Bump zizmorcore/zizmor-action in the github-actions group (#412) Bump https://github.com/astral-sh/ruff-pre-commit (#411)
Fixes #422.
ReadFileChunk.read()passed negative sizes straight to the underlying file.read(-1)returned the rest of the file instead of the rest of the chunk. Boths3transfer.utils.ReadFileChunkand the legacys3transfer.ReadFileChunknow treat a negative size likeNoneand read only to the end of the chunk.The legacy reader also clamps the remaining length at zero after seeking past the chunk boundary, matching
utils.ReadFileChunk. Reads from that position return empty bytes without moving the position or reporting extra bytes read. Seeking back into the chunk still allows normal reads. Theseek()implementation and callback mechanism are unchanged.Added unit regressions for both classes with a nonzero offset, a partial first read, end-of-chunk reads and callback totals. The legacy seek-past-boundary regression covers
None, negative, zero and positive read sizes, position preservation, callbacks and seeking back into the chunk. A functional multipart upload test checks the part contents when its body consumer usesread(-1).Validation on the updated code:
scripts/ci/run-testspasses on both versions. Each has 683 passed / 54 skipped with threaded execution, then 676 passed / 61 skipped with serial execution.No tests were run against live S3. The change only affects read-all/negative-size reads and reads after seeking past the chunk. Normal in-bounds, positive-size reads behave the same as before.