Replace broken upload attributes with new ones

[Dreamsorcerer]

The upload progress attributes added recently fundamentally can't work as most servers won't send the headers in response to a request until they've read the entire body.

This replaces it with a new API.

[codecov]

Codecov Report

❌ Patch coverage is 97.77015% with 13 lines in your changes missing coverage. Please review.
✅ Project coverage is 98.98%. Comparing base (0660260) to head (83cee6c).
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
aiohttp/payload.py	87.50%	11 Missing and 1 partial ⚠️
aiohttp/client_reqrep.py	97.56%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #13427      +/-   ##
==========================================
- Coverage   99.00%   98.98%   -0.02%     
==========================================
  Files         132      132              
  Lines       49531    49996     +465     
  Branches     2571     2593      +22     
==========================================
+ Hits        49036    49490     +454     
- Misses        371      381      +10     
- Partials      124      125       +1     

Flag	Coverage Δ
Autobahn	21.97% <14.57%> (-0.09%)	⬇️
CI-GHA	98.89% <97.77%> (-0.02%)	⬇️
OS-Linux	98.67% <97.77%> (-0.02%)	⬇️
OS-Windows	97.04% <97.77%> (+<0.01%)	⬆️
OS-macOS	97.92% <97.77%> (+<0.01%)	⬆️
Py-3.10	98.12% <97.77%> (-0.01%)	⬇️
Py-3.11	98.36% <96.74%> (-0.02%)	⬇️
Py-3.12	98.46% <97.77%> (+<0.01%)	⬆️
Py-3.13	98.45% <97.77%> (-0.01%)	⬇️
Py-3.14	98.47% <97.77%> (+<0.01%)	⬆️
Py-3.14t	97.56% <97.77%> (-0.01%)	⬇️
Py-pypy-3.11	97.42% <97.77%> (+<0.01%)	⬆️
VM-macos	97.92% <97.77%> (+<0.01%)	⬆️
VM-ubuntu	98.67% <97.77%> (-0.02%)	⬇️
VM-windows	97.04% <97.77%> (+<0.01%)	⬆️
cython-coverage	82.52% <97.77%> (+0.34%)	⬆️

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.

[greptile-apps]

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

_{Reviews (12): Last reviewed commit: "Coverage" | Re-trigger Greptile}

[codspeed-hq]

Merging this PR will not alter performance

✅ 84 untouched benchmarks
⏩ 83 skipped benchmarks¹

---

_{Comparing upload-progress (83cee6c) 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 — Replace broken upload attributes with new ones

Right call on the API replacement, and the implementation is clean — one gap in the abort path keeps it from being merge-ready.

Moving progress tracking from ClientResponse to Payload is the correct fix: the old attributes genuinely couldn't work, since resp only materializes once the server responds, which for most servers is after the body is fully read. Specific things done well here:

• _ProgressWriter counts at the payload boundary, so bytes_written excludes chunked framing and compression — and the rewritten test_payload_upload_progress asserts that exactly (samples == [chunk_size * (i + 1) ...]), which is a much stronger test than the old framing-arithmetic version.
• The lazy-future design in upload_complete handles late access, pre-request access, and post-completion access, and the done() guards in _finish_upload/_abort_upload mean concurrent reuse degrades to wrong numbers rather than an InvalidStateError.
• memoryview.nbytes vs len() is handled correctly for non-contiguous views.
• Deprecation is done properly: stacklevel=2, internal __reset_writer still uses _upload_complete directly so it never trips the repo's filterwarnings = error, and both changelog fragments use valid towncrier types.
• Redirect resend, abort, empty body, and sequential reuse all got dedicated tests.

What needs attention:

• 🟡 upload_complete is only aborted from the finally inside _write_bytes, so any failure before the body write starts — connect refused, DNS, TLS handshake, proxy failure, a header-write OSError — leaves the future pending forever, contradicting the docstring's "cancelled ... e.g. connection error". The docs' own reporter-task example hangs or leaks in that case.
• 🟢 body._finish_upload() in the success branch is missing the track_progress guard, so it mutates the process-wide _EMPTY_BODY singleton on expect100/write-paused bodyless requests.
• 🟢 A reused payload can hand a reporter started before the request the previous upload's already-resolved future (same shared-state concern @greptile-apps flagged; downgraded because it produces wrong numbers, not a crash).
• 🟢 await payload.upload_complete raising CancelledError on abort is undocumented and turns a user's reporter task into a cancelled one.
• 🟢 The proxy adds a coroutine hop per chunk on every client request with a body, tracked or not — worth a benchmark number.
• 🟢 __getattr__ passthrough has no in-tree caller and disables mypy on the proxy.
• 🟢 The deprecated attributes were deleted from docs/client_reference.rst rather than kept with a .. deprecated:: directive, so the migration note never renders.
• 🟢 post_task in test_payload_upload_progress leaks and masks the real error when the sampling loop fails.

---

🟡 Important

1. `upload_complete` never settles when the request fails before the body write starts
aiohttp/payload.py:250-260

The docstring promises the future "is cancelled if the upload is interrupted (e.g. connection error or request cancellation)", but the only place _abort_upload() is called is the finally inside ClientRequest._write_bytes (client_reqrep.py:1557-1560). Any failure that happens before _write_bytes is entered leaves the future pending forever:

• connector failures — DNS resolution, connect refused, TLS handshake, proxy CONNECT — _send() is never reached at all;
• an OSError raised by await writer.write_headers(...) in _send() (client_reqrep.py:979), which happens before the writer task is created;
• the _should_write() false branch if anything raises between writer.set_eof() and _mark_body_sent().

Why it matters: the pattern the new docs teach creates a reporter task up front, so on a connect failure the user is left with either a task that hangs on await payload.upload_complete forever (if they catch the request error and then join the reporter) or a pending task that asyncio destroys with a "Task was destroyed but it is pending" warning. A connect error is exactly the case a progress reporter needs to be told about, and it is the one case that never fires.

Suggested fix: hook the abort into the request teardown that already runs on every failure — ClientSession._request's except BaseException block already does await req._body.close() (client.py:888-896); calling req._body._abort_upload() there (and/or in ClientRequest._terminate()) covers all the pre-write paths. Worth a test that a ClientConnectorError (unroutable host) leaves payload.upload_complete cancelled rather than pending.

    def upload_complete(self) -> "asyncio.Future[None]":
        """Future resolved when a request finishes uploading this payload.

        The future completes with ``None`` once the request body has been
        fully written, or is cancelled if the upload is interrupted (e.g.
        connection error or request cancellation). If the payload is sent
        again (e.g. a redirected request resends the body), a new future
        is returned for the new upload.

        Must be accessed from within the event loop.
        """

🟢 Suggestions

1. `_finish_upload()` is called unconditionally, mutating the shared `_EMPTY_BODY` singleton
aiohttp/client_reqrep.py:1555

The success branch calls body._finish_upload() without the track_progress guard that wraps _start_upload() and _abort_upload(). When the body is ClientRequest._EMPTY_BODY — reachable for a bodyless request with expect100=True or when protocol.writing_paused is set, since _should_write() returns True in those cases — this mutates a class-level payload instance shared by every ClientRequest in the process.

The practical impact is small (nothing else touches that singleton's flags, and _start_upload() is never called on it so _upload_finished sticks at True forever), but it's an asymmetry that will bite whoever next reasons about the state machine, and it means the singleton carries per-request state across event loops.

Move the call under the same guard:

await writer.write_eof()
if track_progress:
    body._finish_upload()
protocol.start_timeout()

            else:
                # Successfully wrote the body, signal EOF and start response timeout
                await writer.write_eof()
                body._finish_upload()
                protocol.start_timeout()

2. Reused payload can hand out the previous upload's already-done future
aiohttp/payload.py:269-279

_start_upload() only clears the stale future when it actually runs, and it runs inside _write_bytes — which on Python < 3.12 is a task scheduled on the next loop iteration. A reporter task started before the request (exactly what the new docs example does) can therefore observe the previous upload's resolved future and exit immediately:

p = aiohttp.BytesPayload(body)
await session.post(url, data=p)          # p.upload_complete resolves

progress = asyncio.create_task(report(p))  # sees the stale done future
async with session.post(url2, data=p):     # reporter already exited
    ...

The same state is shared if a single Payload is used by two overlapping requests (legal today for reusable payloads such as BytesPayload): _bytes_written is reset and incremented by both, and whichever request finishes first resolves the future for both. It cannot raise InvalidStateError — both _finish_upload and _abort_upload guard on fut.done() — so the failure mode is silently wrong numbers rather than a crash, which is why this is a suggestion rather than a blocker.

This is the same concern @greptile-apps raised at payload.py:279. Either document the constraint on Payload.bytes_written / Payload.upload_complete ("progress tracking assumes one in-flight upload per payload instance at a time; reuse in overlapping requests produces undefined counters"), or reset the state earlier — e.g. in ClientRequest.update_body/_send rather than inside the writer task — so the reset is ordered before the caller can observe it.

    def _start_upload(self) -> None:
        """Reset upload progress state before the payload is written."""
        self._bytes_written = 0

3. Deprecated attributes deleted from the docs instead of being marked deprecated
docs/client_reference.rst:1565

ClientResponse.output_size and ClientResponse.upload_complete still exist and still work — they only warn. Removing their .. attribute:: blocks entirely means the .. deprecated:: 3.14.4 Use :attr:Payload.bytes_written instead notes added to the docstrings in client_reqrep.py are never rendered, and a 3.14.x user who hits the DeprecationWarning and searches the docs for output_size finds nothing.

The usual shape is to keep the attribute documented with a .. deprecated:: directive pointing at the replacement, and drop the block only when the attribute is actually removed:

   .. attribute:: output_size

      Number of bytes sent for this request.

      .. deprecated:: 3.14.4

         Cannot work reliably — most servers do not respond until the
         whole body is read. Use :attr:`Payload.bytes_written` instead.

That also gives the changelog's deprecation entry somewhere to link to.

4. `post_task` leaks and masks the real error if the sampling loop fails
tests/test_client_functional.py:6106-6115

Two hygiene issues in the rewritten progress test:

• If any assertion inside the for loop fails (or the loop hangs on next_chunk.wait() because the upload died early), post_task is never awaited or cancelled — pytest reports the assertion, then asyncio emits "Task was destroyed but it is pending" and the connection is left dangling, which can bleed into neighbouring tests under --numprocesses=auto.
• On any upload failure, await p.upload_complete raises CancelledError (the future is cancelled by _abort_upload), so the test fails with a bare cancellation instead of the underlying ClientError that post_task holds. Awaiting post_task first, or wrapping the body in try/finally: post_task.cancel(), gives a debuggable failure.

A try: ... finally: post_task.cancel(); with suppress(CancelledError): await post_task around the sampling loop covers both.

Separately: the github-code-quality bot's "Statement has no effect" on this line is a false positive — await post_task both joins the task and re-raises any exception it captured, which is the whole point of the line.

    post_task = asyncio.create_task(do_post())
    samples: list[int] = []
    for _ in range(num_chunks):
        await next_chunk.wait()
        next_chunk.clear()
        samples.append(p.bytes_written)
        assert not p.upload_complete.done()
        sample_taken.set()
    await p.upload_complete
    await post_task

---

Checklist

• No hardcoded secrets or unsafe operations
• Error paths settle all observable state — warning ＃1, suggestion ＃1
• No resource leaks or dangling tasks — warning ＃1, suggestion ＃4
• Public API contract matches documented behaviour — warning ＃1, suggestion ＃3
• Backward compatibility handled (deprecation, not removal)
• Changelog fragments present and valid types
• New behaviour covered by tests — warning ＃1, suggestion ＃4
• No shared-state / concurrency hazards — suggestion ＃1, suggestion ＃2
• Diff matches PR description (no scope creep)

---

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.)

---

Silent Failure Analysis

🟠 **HIGH** — failure path never reached — future left pending forever
aiohttp/client_reqrep.py:1507-1566

Risk: _start_upload/_abort_upload only ever run inside _write_bytes, so any failure before it (connector/DNS/TLS errors, await writer.write_headers() raising, request aborted before _should_write) leaves a Payload.upload_complete future that was accessed pre-request pending forever — the documented while not p.upload_complete.done() reporter task then spins indefinitely with no error ever surfacing through the payload API.

body._start_upload()
writer = payload._ProgressWriter(writer, body)
try:
    ...
finally:
    if track_progress:
        body._abort_upload()

Fix: Abort upload tracking from the request-level error path too (e.g. in ClientRequest._terminate/_close or in _send's failure handling), so the future is always settled once the request is finished.

🟡 **MEDIUM** — error signalled by cancellation, losing the cause
aiohttp/payload.py:288-295

Risk: An upload failure is reported as a bare CancelledError carrying none of the ClientOSError/ClientConnectionError context that _write_bytes stashed on the protocol (that branch does not re-raise), and it is indistinguishable from a caller-initiated cancel — the very pattern the new docs recommend (progress.cancel() + contextlib.suppress(asyncio.CancelledError)) swallows a genuine upload failure entirely; unlike set_exception, a dropped cancelled future also produces no "exception was never retrieved" warning.

def _abort_upload(self) -> None:
    if self._upload_finished:
        return
    self._upload_aborted = True
    fut = self._upload_future
    if fut is not None and not fut.done():
        fut.cancel()

Fix: Set the underlying exception on the future (fut.set_exception(...)) instead of cancelling it, or expose the abort reason on the payload so consumers can distinguish failure from cancellation.

🟡 **MEDIUM** — silent no-op discards a failure
aiohttp/payload.py:288-291

Risk: Upload state is stored on the shared Payload object, so when the same reusable payload (e.g. BytesPayload) is sent by two concurrent requests, one request's _finish_upload() makes the other's _abort_upload() an unconditional no-op — the failed upload is silently reported as complete, and bytes_written is corrupted by interleaved _start_upload() resets.

def _abort_upload(self) -> None:
    if self._upload_finished:
        return

Fix: Key the progress state per in-flight upload (e.g. a per-request token checked in _finish_upload/_abort_upload) or explicitly reject/warn on concurrent uploads of the same payload.

🟡 **MEDIUM** — stale error state not reset — reports failure for a successful upload
aiohttp/client_reqrep.py:1472-1474

Risk: This path calls _finish_upload() without _start_upload(), so _upload_aborted from a previous failed attempt is never cleared; since upload_complete checks _upload_aborted before _upload_finished, a reused zero-length payload whose earlier expect100 attempt was aborted returns a cancelled future (or leaves the old cancelled one) even though this request succeeded.

def _mark_body_sent(self) -> None:
    if (body := self._body) is not self._EMPTY_BODY:
        body._finish_upload()

Fix: Call body._start_upload() before body._finish_upload() here, and/or clear _upload_aborted in _finish_upload().

🟡 **MEDIUM** — fire-and-forget task — failure becomes a hang
tests/test_client_functional.py:6100-6115

Risk: If do_post() raises (connection error, non-200, assertion failure) the next_chunk event is never set and the test blocks forever on next_chunk.wait() instead of failing, with the task's exception surfacing only as "Task exception was never retrieved" at GC.

post_task = asyncio.create_task(do_post())
samples: list[int] = []
for _ in range(num_chunks):
    await next_chunk.wait()

Fix: Wrap the sampling loop in asyncio.timeout(...) or await the gated waits together with post_task (e.g. asyncio.wait([...], return_when=FIRST_COMPLETED) so a failed request fails the test.

---

Automated review by Kōan (Claude) HEAD=e3db1ad 8 min 55s

[greptile-apps]

The review is thorough and the findings are accurate. Having read the diff:

The abort path gap is the real blocker. _abort_upload() only fires from the finally in _write_bytes, which means any failure in the connector layer (DNS, TCP connect, TLS, proxy CONNECT) leaves the future pending indefinitely. The docstring and docs example both promise cancellation on connection errors — that promise isn't kept. The suggested fix of hooking into ClientSession._request's except BaseException block (or ClientRequest._terminate()) is the right approach.

The _EMPTY_BODY singleton mutation is confirmed. In the else branch of _write_bytes:

else:
    await writer.write_eof()
    body._finish_upload()   # ← no track_progress guard
    protocol.start_timeout()

_finish_upload() will be called on _EMPTY_BODY for bodyless expect100 or write-paused requests since track_progress = body is not self._EMPTY_BODY is False in those cases. Contrast this with _mark_body_sent in ClientDataRequest, which already has the guard:

def _mark_body_sent(self) -> None:
    if (body := self._body) is not self._EMPTY_BODY:
        body._finish_upload()

The fix is a one-liner — wrap with if track_progress: in _write_bytes to match.

post_task leak in the test is real. If the gated-body assertion fails mid-loop, post_task is never cancelled, and the next_chunk.wait() inside the task hangs event loop teardown. A try/finally: post_task.cancel(); await asyncio.gather(post_task, return_exceptions=True) wrapper around the sampling loop is the clean fix.

One additional note not called out above: _start_upload() only clears the stale future when fut.done() is True — but if upload_complete was accessed before _start_upload() runs for the current upload and the previous future is somehow still pending, the pending future is kept and both uploads share it. This is unlikely in practice since _abort_upload() in the finally ensures the previous future is always settled before the next request runs, but it's another argument for resetting state earlier (in update_body/_send) rather than inside the writer task.

In summary: address the abort path (🟡) and the track_progress guard on _finish_upload (🟢 #1) before merging. The rest are polish items.

[greptile-apps]

The review is accurate. Here's confirmation against the diff for the two most actionable gaps:

Critical issue is real. _abort_upload() is only called in the finally of _write_bytes. The entire _write_bytes method is only reached if _should_write() returns True and the writer task is scheduled. Any failure in _send() before that — DNS, TLS, proxy CONNECT, or write_headers raising — leaves the future pending forever. The docstring's guarantee is broken for the most common failure modes.

Suggestion #1 is also valid. In the _write_bytes success branch:

else:
    await writer.write_eof()
    body._finish_upload()   # no track_progress guard
    protocol.start_timeout()

_write_bytes is entered when _should_write() is true — which includes expect100=True even for zero-length bodies. When body is _EMPTY_BODY, track_progress is False, but _finish_upload() still runs on the shared singleton, setting _upload_finished = True as an instance attribute on the class-level object. The fix is correct: move body._finish_upload() under the if track_progress: guard.

One nuance the review understates: _mark_body_sent (client_reqrep.py:1472-1474) already has the is not self._EMPTY_BODY guard correctly — so the singleton mutation is specific to the _write_bytes success branch, an inconsistency between the two completion paths rather than a universal problem.

The priority ordering is right: fix the pending-forever future first (it makes the documented API unreliable under the most common error condition), then the singleton mutation. The remaining suggestions are genuine polish but not blockers.