Reduce number of file descriptors needed in multipart messages

[Dreamsorcerer]

No description provided.

[greptile-apps]

Confidence Score: 5/5

The PR appears safe to merge because no blocking failure remains.

No blocking failure remains.

_{Reviews (8): Last reviewed commit: "Merge branch 'fix-multipart-fds' of gith..." | Re-trigger Greptile}

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.00%. Comparing base (0660260) to head (0baf016).
✅ All tests successful. No failed tests found.

Additional details and impacted files

@@           Coverage Diff           @@
##           master   #13426   +/-   ##
=======================================
  Coverage   99.00%   99.00%           
=======================================
  Files         132      132           
  Lines       49531    49626   +95     
  Branches     2571     2575    +4     
=======================================
+ Hits        49036    49131   +95     
  Misses        371      371           
  Partials      124      124           

Flag	Coverage Δ
Autobahn	22.04% <15.09%> (-0.02%)	⬇️
CI-GHA	98.91% <100.00%> (+<0.01%)	⬆️
OS-Linux	98.68% <100.00%> (+<0.01%)	⬆️
OS-Windows	97.01% <87.85%> (-0.03%)	⬇️
OS-macOS	97.93% <100.00%> (+0.01%)	⬆️
Py-3.10	98.13% <99.06%> (+<0.01%)	⬆️
Py-3.11	98.36% <92.52%> (-0.02%)	⬇️
Py-3.12	98.45% <92.52%> (-0.01%)	⬇️
Py-3.13	98.44% <92.52%> (-0.01%)	⬇️
Py-3.14	98.45% <92.45%> (-0.02%)	⬇️
Py-3.14t	97.55% <92.45%> (-0.02%)	⬇️
Py-pypy-3.11	97.40% <92.52%> (-0.02%)	⬇️
VM-macos	97.93% <100.00%> (+0.01%)	⬆️
VM-ubuntu	98.68% <100.00%> (+<0.01%)	⬆️
VM-windows	97.01% <87.85%> (-0.03%)	⬇️
cython-coverage	82.24% <92.52%> (+0.06%)	⬆️

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

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

[codspeed-hq]

Merging this PR will not alter performance

✅ 84 untouched benchmarks
⏩ 83 skipped benchmarks¹

---

_{Comparing fix-multipart-fds (0baf016) with master (0660260)}

Footnotes

1. 83 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

[aiolibsbot]

PR Review — Reduce number of file descriptors needed in multipart messages

Good fix for the descriptor-exhaustion problem, with genuinely well-chosen tests — but the user-visible side effects (limit semantics, FileField.file concrete type) need to be written down before this ships.

Specific things done well:

• The SpooledTemporaryFile shim for < 3.11 is correct and necessary. I checked it on 3.10.20: tempfile.SpooledTemporaryFile genuinely lacks readable/writable/seekable there and is not an io.IOBase subclass, [bytes] subscripting does work at runtime (__class_getitem__ exists in 3.10), and the two-base class definition works with no layout conflict across the spool/rollover boundary.
• Switching to payload.total_bytes removes the double-accounting bug where framing bytes were invisible to the limit, and it lines multipart up with BaseRequest.read(), which already checks raw body bytes.
• Rollover still happens inside run_in_executor (via tmp.write), so the blocking open() did not silently move onto the event loop when the explicit run_in_executor(None, tempfile.TemporaryFile) was dropped.
• test_post_file_fields_descriptor_cost is a real regression test for the actual property being fixed, not a proxy for it, and test_post_file_field_spool_rollover parametrised on size-1 / size / size+1 pins byte-identity across the rollover boundary and exercises the 3.10 shim's three predicates.
• I checked the obvious hole — a Content-Encoding: gzip part now escaping a decoded-size cap — and it is not reachable: BodyPartReader._needs_content_decoding() is gated on not self._is_form_data, so post() never decompresses. Only Content-Transfer-Encoding applies, and base64/quoted-printable can only shrink, so raw bytes remain a sound upper bound.

What needs attention:

• 🟡 client_max_size now counts boundaries and part headers — a 1 MiB file against the 1 MiB default that passed yesterday now 413s. The behaviour looks right; it just needs a changelog line and a docs update (web_reference.rst:1508, web_quickstart.rst:492, which still claims files always go to a temp directory).
• 🟡 FileField.file is still annotated and cast as io.BufferedReader, which a SpooledTemporaryFile is not: .peek()/.raw are missing on every supported version, and .read1()/.readinto()/.detach() are missing on 3.10. mypy will keep blessing calls that now raise AttributeError.
• 🟢 THREAT_MODEL.md §5.4 threat 4.14 and its mitigation still describe unconditional tempfile.TemporaryFile() streaming; AGENTS.md makes updating this mandatory. Worth recording the fd-for-memory trade-off there too.
• 🟢 The new top-of-loop guard's comment says "empty payloads" when it means parts with empty bodies, and its placement leaves the closing delimiter outside the limit.

Note that the author has three pending deletion suggestions on the test file (lines 2167, 2186, 2244) that are not yet applied — I have reviewed the diff as posted.

---

🟡 Important

1. `client_max_size` semantics change for multipart is undocumented
aiohttp/web_request.py:771-776

The limit is now measured against self._payload.total_bytes (raw bytes fed to the request stream) instead of the old size accumulator, which summed only the decoded content of the parts.

That is a real behaviour change on a public knob, and I think the change itself is right — it makes multipart consistent with BaseRequest.read(), which already checks len(body) > client_max_size on raw body bytes (aiohttp/web_request.py:672) — but it is currently invisible to users:

• With the default client_max_size=1024**2, a form uploading a file of exactly 1 MiB used to pass (0 < max_size < size is strict, and size excluded framing). It now 413s, because total_bytes additionally includes the boundary lines, the per-part Content-Disposition headers and the CRLFs. Same for anything within ~a few hundred bytes of the limit.
• The same applies to Content-Transfer-Encoding: base64 parts, which now count 4/3 of their decoded size — your own test_post_max_client_size_counts_undecoded_bytes pins that.

Real-world impact: a deployment that upgrades within a patch series starts rejecting uploads that worked yesterday, and nothing in the release notes explains why.

Suggested fix — no code change needed, just make it visible:

• Add a second changelog fragment (CHANGES/13426.bugfix.1.rst or a .misc) stating that client_max_size now applies to the whole raw multipart body including boundaries and part headers.
• Update docs/web_reference.rst:1508 (:param client_max_size:) and the docs/web_quickstart.rst:492 paragraph, which still says post "stores files data in temporary directory" — with a 1 MiB spool and a 1 MiB default limit, the default case now never touches disk at all.

            payload = self._payload
            while (field := await multipart.next()) is not None:
                if 0 < max_size < payload.total_bytes:
                    raise HTTPRequestEntityTooLarge(max_size)

2. `FileField.file` is no longer a `BufferedReader`, but still cast/annotated as one
aiohttp/web_request.py:808

FileField.file is annotated io.BufferedReader (aiohttp/web_request.py:92) and the value is still cast(io.BufferedReader, tmp). Before this PR that cast was truthful — tempfile.TemporaryFile() returns a BufferedRandom, which is a BufferedReader subclass. It is now a SpooledTemporaryFile, which is not.

I verified the concrete API gap on both ends of the support range:

• Python 3.10 and 3.12+: peek and raw do not exist on SpooledTemporaryFile.
• Python 3.10 additionally lacks read1, readinto and detach (they were only added alongside the io.IOBase base in 3.11).

Why it matters: user code that does data["upload"].file.peek(...) or .readinto(buf) — perfectly legal against the declared type — now raises AttributeError at runtime, and mypy will not warn because the cast asserts a type the object no longer has. That is a silent break shipped under a bugfix note.

Suggested fix: widen the field to something honest and drop the lie in the cast. The Sphinx docs already promise the weaker contract — docs/web_reference.rst:3075 says "An io.IOBase instance with content of uploaded file" — so io.IOBase (or IO[bytes]) matches the documented behaviour and is what your new shim already guarantees on 3.10. Mention the concrete-type change in the changelog fragment so downstream users can grep for .peek()/.raw usage.

                            field_ct,

🟢 Suggestions

1. THREAT_MODEL.md §5.4 now describes behaviour that no longer exists
aiohttp/web_request.py:790

AGENTS.md lists THREAT_MODEL.md as a living document that must be revised when a referenced default changes. Two entries in §5.4 are now factually wrong:

• Threat 4.14 (THREAT_MODEL.md:654): "Uploaded files are streamed to tempfile.TemporaryFile()" — they are now buffered in memory up to 1 MiB first.
• Mitigation 4.14 (THREAT_MODEL.md:673): "tempfile.TemporaryFile() is unlinked at creation on POSIX; closing the FD ... reclaims the disk" — for the default configuration no FD and no disk object is ever created, so the whole mitigation argument no longer applies.

Worth capturing the trade-off explicitly while you are in there, since it is the interesting part of this change: descriptors per request drop to client_max_size // _FILE_SPOOL_MAX_SIZE, but resident memory for file parts rises from ~O(chunk) to up to client_max_size (N parts of just under 1 MiB each all stay in RAM). For the 1 MiB default that is a wash and it already matches what the non-file branch does with its raw_data bytearray — but an application that raised client_max_size to, say, 1 GiB now holds up to 1 GiB per in-flight request in memory where it previously held almost none. That is a deliberate choice, it just should be written down next to 4.2/4.5 rather than discovered in production.

(This overlaps with @greptile-apps' comment on line 789 — flagging it because the repo's own AGENTS.md makes it a hard requirement rather than a preference.)

                        tmp = SpooledTemporaryFile(_FILE_SPOOL_MAX_SIZE)

2. Top-of-loop placement leaves the trailing framing unchecked; comment is misleading
aiohttp/web_request.py:773-776

Two small things about this guard:

• The comment says "needed for empty payloads, which still add overhead without entering the loop". Read literally that describes an empty request body, which never enters while (field := ...) at all and so is never checked here. What it actually guards is a part with an empty body, whose inner while chunk := await field.read_chunk(...) loop never runs. Rewording to "parts with empty bodies" would save the next reader the double-take.

• Because the check sits at the top of the iteration, the framing consumed after the final part (the closing --b-- delimiter) is never measured. So client_max_size is enforced on "everything up to and including the last part's body" rather than on the whole body — a small inconsistency with the new raw-bytes semantics from finding 1. A single check after the loop exits would close it.

Neither is a live bug; both are cheap to fix while the code is fresh.

                # This check is needed for empty payloads, which still add
                # overhead without entering the loop and the check below.
                if 0 < max_size < payload.total_bytes:
                    raise HTTPRequestEntityTooLarge(max_size)

---

Checklist

• New behaviour covered by tests
• Python 3.10 compatibility of the version-gated shim verified
• No resource leak or unbounded growth introduced
• Blocking I/O stays off the event loop
• No decompression-bomb regression from dropping decoded-size accounting
• Public API types match runtime reality — warning ＃2
• User-visible behaviour change documented (changelog + docs) — warning ＃1
• THREAT_MODEL.md revised per AGENTS.md triggers — suggestion ＃1
• Changelog fragment present and correctly signed

---

To rebase and address feedback, mention me: @aiolibsbot rebase critical (fixes 🔴 only), @aiolibsbot rebase important (fixes 🔴 + 🟡), or @aiolibsbot rebase --fix for all. (A bare @aiolibsbot rebase only rebases onto the base branch.)

---

Automated review by Kōan (Claude) HEAD=f2d3e2e 8 min 52s

[greptile-apps]

Thanks for sharing the full Kōan review summary. It looks like a thorough automated pass — is there a specific point from it you'd like to dig into, or would you like me to look at a particular part of the diff in more detail?

For example, I can:

• Pull the current FileField annotation and the SpooledTemporaryFile shim to verify the type gap in finding improve README.rst #2
• Check the THREAT_MODEL.md entries that need updating per suggestion API changed to create_server #1
• Look at the loop placement around line 773 for the comment/placement issue in suggestion improve README.rst #2

What would be most useful?

Tip: You can customize Greptile's behavior for this repo with .greptile/rules.md and .greptile/config.json.

[Dreamsorcerer]

Weird that codspeed said there was a 23% improvement on one benchmark originally, and now it's disappeared with the executor change (which I'd have expected to improve performance further).

[patchback]

Backport to 3.15: 💔 cherry-picking failed — conflicts found

❌ Failed to cleanly apply c92ed4c on top of patchback/backports/3.15/c92ed4c48dafd40d5ed4c86587af9e081ac7d971/pr-13426

Backporting merged PR #13426 into master

1. Ensure you have a local repo clone of your fork. Unless you cloned it
   from the upstream, this would be your origin remote.
2. Make sure you have an upstream repo added as a remote too. In these
   instructions you'll refer to it by the name upstream. If you don't
   have it, here's how you can add it:

   $ git remote add upstream https://github.com/aio-libs/aiohttp.git

3. Ensure you have the latest copy of upstream and prepare a branch
   that will hold the backported code:

   $ git fetch upstream
   $ git checkout -b patchback/backports/3.15/c92ed4c48dafd40d5ed4c86587af9e081ac7d971/pr-13426 upstream/3.15

4. Now, cherry-pick PR Reduce number of file descriptors needed in multipart messages #13426 contents into that branch:

   $ git cherry-pick -x c92ed4c48dafd40d5ed4c86587af9e081ac7d971

   If it'll yell at you with something like fatal: Commit c92ed4c48dafd40d5ed4c86587af9e081ac7d971 is a merge but no -m option was given., add -m 1 as follows instead:

   $ git cherry-pick -m1 -x c92ed4c48dafd40d5ed4c86587af9e081ac7d971

5. At this point, you'll probably encounter some merge conflicts. You must
   resolve them in to preserve the patch from PR Reduce number of file descriptors needed in multipart messages #13426 as close to the
   original as possible.
6. Push this branch to your fork on GitHub:

   $ git push origin patchback/backports/3.15/c92ed4c48dafd40d5ed4c86587af9e081ac7d971/pr-13426

7. Create a PR, ensure that the CI is green. If it's not — update it so that
   the tests and any other checks pass. This is it!
   Now relax and wait for the maintainers to process your pull request
   when they have some cycles to do reviews. Don't worry — they'll tell you if
   any improvements are necessary when the time comes!

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

[patchback]

Backport to 3.14: 💔 cherry-picking failed — conflicts found

❌ Failed to cleanly apply c92ed4c on top of patchback/backports/3.14/c92ed4c48dafd40d5ed4c86587af9e081ac7d971/pr-13426

Backporting merged PR #13426 into master

1. Ensure you have a local repo clone of your fork. Unless you cloned it
   from the upstream, this would be your origin remote.
2. Make sure you have an upstream repo added as a remote too. In these
   instructions you'll refer to it by the name upstream. If you don't
   have it, here's how you can add it:

   $ git remote add upstream https://github.com/aio-libs/aiohttp.git

3. Ensure you have the latest copy of upstream and prepare a branch
   that will hold the backported code:

   $ git fetch upstream
   $ git checkout -b patchback/backports/3.14/c92ed4c48dafd40d5ed4c86587af9e081ac7d971/pr-13426 upstream/3.14

4. Now, cherry-pick PR Reduce number of file descriptors needed in multipart messages #13426 contents into that branch:

   $ git cherry-pick -x c92ed4c48dafd40d5ed4c86587af9e081ac7d971

   If it'll yell at you with something like fatal: Commit c92ed4c48dafd40d5ed4c86587af9e081ac7d971 is a merge but no -m option was given., add -m 1 as follows instead:

   $ git cherry-pick -m1 -x c92ed4c48dafd40d5ed4c86587af9e081ac7d971

5. At this point, you'll probably encounter some merge conflicts. You must
   resolve them in to preserve the patch from PR Reduce number of file descriptors needed in multipart messages #13426 as close to the
   original as possible.
6. Push this branch to your fork on GitHub:

   $ git push origin patchback/backports/3.14/c92ed4c48dafd40d5ed4c86587af9e081ac7d971/pr-13426

7. Create a PR, ensure that the CI is green. If it's not — update it so that
   the tests and any other checks pass. This is it!
   Now relax and wait for the maintainers to process your pull request
   when they have some cycles to do reviews. Don't worry — they'll tell you if
   any improvements are necessary when the time comes!

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