Skip to content

Conversation

bdraco
Copy link
Member

@bdraco bdraco commented Jul 9, 2025

What do these changes do?

These changes fix a bug where file uploads fail with HTTP 422 errors when encountering 307/308 redirects. The issue occurs because:

  1. HTTP 307/308 redirects preserve the request body (unlike 301/302/303)
  2. When aiohttp reuses the same file payload for the redirect, the file position is at EOF after the first request
  3. This causes the Content-Length calculation to return 0 for the second request, leading to server rejection

The fix includes:

  • Client change: Preserve the payload instance (not raw data) for 307/308 redirects to prevent recreation of consumed payloads
  • Payload change: Store initial file position on first access and calculate size from that position, ensuring correct Content-Length for reused payloads

Are there changes in behavior for the user?

File uploads that encounter 307/308 redirects will now work correctly instead of failing with HTTP 422 errors. This is a bug fix that restores expected behavior - no breaking changes.

Is it a substantial burden for the maintainers to support this?

No. The changes are minimal and focused:

  • Simple logic to preserve payload instances for specific redirect types
  • File position management that's already part of the payload lifecycle
  • Well-tested with both unit and integration tests
  • No new APIs or complex abstractions

Related issue number

Fixes #11270

Implementation Details

Changes Made

  1. aiohttp/client.py

    • Modified redirect handling to preserve payload instance for 307/308 redirects
    • Changed from data = req.data to data = req._body to avoid payload recreation
  2. aiohttp/payload.py

    • Modified size property to store start position on first access
    • Returns total size from start position instead of remaining bytes
    • Added AttributeError handling for tarfile streams
    • Properly handles unseekable files by returning None for size
  3. tests/test_payload.py

    • Added test for payload size calculation after reading
    • Added test for unseekable file handling
  4. tests/test_client_functional.py

    • Added integration test for file upload with 307 redirect

Technical Explanation

The root cause identified by Cycloctane in the issue: "Two requests share the same BufferedReaderPayload instance, while the second request get zero Content-Length."

This happens because:

  1. First request reads the file, moving position to EOF
  2. For 307/308 redirects, the same payload is reused
  3. Size calculation was file_size - current_position, which equals 0 at EOF
  4. Server receives Content-Length: 0 and returns 422 Unprocessable Entity

The fix stores the initial file position when size is first accessed, then always calculates size from that position. This ensures consistent Content-Length across multiple uses of the same payload instance.

Checklist

  • I think the code is well written
  • Unit tests for the changes exist
  • Documentation reflects the changes (no user-facing API changes)
  • Add yourself to CONTRIBUTORS.txt
  • Add a new news fragment 11270.bugfix.rst into the CHANGES/ folder

@psf-chronographer psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Jul 9, 2025
@bdraco bdraco added backport-3.12 Trigger automatic backporting to the 3.12 release branch by Patchback robot backport-3.13 Trigger automatic backporting to the 3.13 release branch by Patchback robot labels Jul 9, 2025
Copy link

codecov bot commented Jul 9, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.84%. Comparing base (9b0153c) to head (9e40b5f).
Report is 3 commits behind head on master.

✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff            @@
##           master   #11290    +/-   ##
========================================
  Coverage   98.84%   98.84%            
========================================
  Files         131      131            
  Lines       43257    43433   +176     
  Branches     2326     2331     +5     
========================================
+ Hits        42756    42933   +177     
  Misses        346      346            
+ Partials      155      154     -1     
Flag Coverage Δ
CI-GHA 98.74% <100.00%> (+<0.01%) ⬆️
OS-Linux 98.47% <100.00%> (+<0.01%) ⬆️
OS-Windows 96.81% <100.00%> (+0.02%) ⬆️
OS-macOS 97.69% <100.00%> (+0.01%) ⬆️
Py-3.10.11 97.33% <100.00%> (+0.01%) ⬆️
Py-3.10.18 97.82% <100.00%> (+<0.01%) ⬆️
Py-3.11.13 98.01% <100.00%> (+<0.01%) ⬆️
Py-3.11.9 97.53% <100.00%> (+0.01%) ⬆️
Py-3.12.10 97.63% <100.00%> (+<0.01%) ⬆️
Py-3.12.11 98.11% <100.00%> (+0.01%) ⬆️
Py-3.13.3 98.37% <100.00%> (+<0.01%) ⬆️
Py-3.9.13 97.24% <100.00%> (+0.02%) ⬆️
Py-3.9.23 97.70% <100.00%> (+<0.01%) ⬆️
Py-pypy7.3.16 85.45% <100.00%> (+8.98%) ⬆️
VM-macos 97.69% <100.00%> (+0.01%) ⬆️
VM-ubuntu 98.47% <100.00%> (+<0.01%) ⬆️
VM-windows 96.81% <100.00%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link

codspeed-hq bot commented Jul 9, 2025

CodSpeed Performance Report

Merging #11290 will not alter performance

Comparing fix_redirect_upload_422 (9e40b5f) with master (e38220f)

Summary

✅ 59 untouched benchmarks

@bdraco bdraco marked this pull request as ready for review July 9, 2025 21:51
@bdraco bdraco requested review from webknjaz and asvetlov as code owners July 9, 2025 21:51
@Dreamsorcerer Dreamsorcerer merged commit 16703bb into master Jul 10, 2025
40 checks passed
@Dreamsorcerer Dreamsorcerer deleted the fix_redirect_upload_422 branch July 10, 2025 10:58
Copy link
Contributor

patchback bot commented Jul 10, 2025

Backport to 3.12: 💚 backport PR created

✅ Backport PR branch: patchback/backports/3.12/16703bb955ae4a11a131cedbbbf3ec7aa55f4bb4/pr-11290

Backported as #11296

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

Copy link
Contributor

patchback bot commented Jul 10, 2025

Backport to 3.13: 💚 backport PR created

✅ Backport PR branch: patchback/backports/3.13/16703bb955ae4a11a131cedbbbf3ec7aa55f4bb4/pr-11290

Backported as #11297

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

@mberdyshev
Copy link

@Dreamsorcerer, as you are getting to merge it to the version branches, can you check my comments above? They are not so significant, though.

Dreamsorcerer pushed a commit that referenced this pull request Jul 10, 2025
…P 422 on 307/308 redirects (#11297)

**This is a backport of PR #11290 as merged into master
(16703bb).**

---------

Co-authored-by: J. Nick Koston <nick@koston.org>
Dreamsorcerer pushed a commit that referenced this pull request Jul 10, 2025
…P 422 on 307/308 redirects (#11296)

**This is a backport of PR #11290 as merged into master
(16703bb).**

---------

Co-authored-by: J. Nick Koston <nick@koston.org>
@Dreamsorcerer
Copy link
Member

Feel free to make a PR with the changes, otherwise bdraco will likely review them later in the day.

patchback bot pushed a commit that referenced this pull request Jul 15, 2025
patchback bot pushed a commit that referenced this pull request Jul 15, 2025
Dreamsorcerer pushed a commit that referenced this pull request Jul 15, 2025
… comments from PR #11290 (#11319)

**This is a backport of PR #11301 as merged into master
(815901f).**

Co-authored-by: J. Nick Koston <nick@koston.org>
Dreamsorcerer pushed a commit that referenced this pull request Jul 15, 2025
… comments from PR #11290 (#11318)

**This is a backport of PR #11301 as merged into master
(815901f).**

Co-authored-by: J. Nick Koston <nick@koston.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-3.12 Trigger automatic backporting to the 3.12 release branch by Patchback robot backport-3.13 Trigger automatic backporting to the 3.13 release branch by Patchback robot bot:chronographer:provided There is a change note present in this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

File multipart upload doesn't work with redirects getting 422 status code
3 participants