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 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 377 commits behind head on master.

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 asvetlov and webknjaz 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 <[email protected]>
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 <[email protected]>
@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 <[email protected]>
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 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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