UploadTracker

[Dreamsorcerer]

No description provided.

[greptile-apps]

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains; the current code fixes both previously reported issues.

_{Reviews (4): Last reviewed commit: "Merge branch 'upload-tracker' of github...." | Re-trigger Greptile}

[greptile-apps]

Threat model omits UploadTracker

This adds a public client request option without updating THREAT_MODEL.md, leaving the security model's public API inventory and required audit trail out of sync with the newly exposed upload-observation surface.

Context Used: AGENTS.md (source)

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

[codecov]

Codecov Report

❌ Patch coverage is 99.55621% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 99.03%. Comparing base (20acdf4) to head (f848f01).
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
tests/test_client_functional.py	99.10%	3 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff            @@
##           master   #13579    +/-   ##
========================================
  Coverage   99.02%   99.03%            
========================================
  Files         135      135            
  Lines       50500    51057   +557     
  Branches     2652     2678    +26     
========================================
+ Hits        50007    50562   +555     
- Misses        370      373     +3     
+ Partials      123      122     -1     

Flag	Coverage Δ
Autobahn	21.92% <13.92%> (-0.12%)	⬇️
CI-GHA	98.92% <99.55%> (+<0.01%)	⬆️
OS-Linux	98.70% <99.55%> (+0.01%)	⬆️
OS-Windows	97.11% <99.55%> (+0.02%)	⬆️
OS-macOS	97.98% <99.55%> (+0.01%)	⬆️
Py-3.10	98.10% <98.07%> (-0.01%)	⬇️
Py-3.11	98.36% <99.55%> (+0.01%)	⬆️
Py-3.12	98.44% <99.55%> (+0.01%)	⬆️
Py-3.13	98.42% <99.55%> (+<0.01%)	⬆️
Py-3.14	98.46% <99.55%> (+0.01%)	⬆️
Py-3.14t	97.63% <99.55%> (+0.02%)	⬆️
Py-pypy-3.11	97.42% <99.55%> (+0.02%)	⬆️
VM-macos	97.98% <99.55%> (+0.01%)	⬆️
VM-ubuntu	98.70% <99.55%> (+0.01%)	⬆️
VM-windows	97.11% <99.55%> (+0.02%)	⬆️
cython-coverage	83.37% <96.26%> (+0.29%)	⬆️

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

✅ 96 untouched benchmarks
⏩ 83 skipped benchmarks¹

---

_{Comparing upload-tracker (f848f01) with master (20acdf4)}

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

[Dreamsorcerer]

@aiolibsbot review

[aiolibsbot]

PR Review — UploadTracker

Solid, well-thought-out API with unusually thorough state-machine testing — one real gap where upload_complete can hang forever, plus nits.

Specific strengths: the generation-token design (_attempt_started() returning a monotonic gen, guarded in _add_bytes/_attempt_finished/_attempt_failed/_attempt_cancelled) correctly neutralises events from a stale writer that is still tearing down while a redirect resends the body — and it is directly unit-tested in test_upload_tracker_stale_attempt_events_ignored. The _finalize()/_settle() split, which defers settling when the final attempt is still ACTIVE, is exactly right for the early-response case and is covered end-to-end. Coverage of the awkward paths (303 body-drop, middleware short-circuit vs. middleware resend, external future cancellation, trace-start failure, invalid proxy) is better than most feature PRs get. Reporting body bytes before compression and chunk framing is a genuine improvement over output_size, and the pre-existing timer leak on a failing on_request_start trace is fixed as a side effect of the _request restructure.

All 20 upload-tracker tests plus test_client_request.py, test_client_session.py, test_client_functional.py and test_http_writer.py pass locally (616 passed, pure-Python mode). greptile's earlier P1 on clipped chunks inflating bytes_written is resolved by the "Fix clipped sizes" commit and covered by test_on_body_write_clipped_chunk — not re-raised.

• 🟡 _bind() sits after the data+json / ssl / session-closed / JsonPayload validation, so those failures never reach _finalize() and upload_complete stays pending forever — verified; this hangs the polling loop the docs recommend.
• 🟢 _settle() only consumes the future's exception when it is the same object the request raised, so the documented while not ...done() pattern triggers a "Future exception was never retrieved" traceback — reproduced.
• 🟢 Moving the 100-continue preamble inside the try now calls conn.close() on cancellation, which fixes a real connection-reuse corruption on master (400 Bad HTTP method in status line 'P/1.1') — but ships with no changelog fragment and no regression test.
• 🟢 The deprecated output_size/upload_complete doc entries were deleted rather than marked .. deprecated::, against this file's own convention.
• 🟢 No test pins the pre-compression semantics of bytes_written (compress=True), the one case where the number intentionally differs from the wire.
• 🟢 Signalling "never sent" by cancelling the future is silently swallowed by asyncio.TaskGroup; worth settling the API shape before release.
• 🟢 THREAT_MODEL.md not revised, which AGENTS.md asks for when public API is added to client.py.

---

🟡 Important

1. Argument-validation failures leave `upload_complete` pending forever
aiohttp/client.py:529-530

_bind() runs after the session-closed check, the ssl type check, the data+json conflict check, and JsonPayload(...) construction. Any of those raising means _finalize() is never reached, so upload_complete is left pending forever.

This contradicts the contract you documented in docs/client_reference.rst ("cancelled when the body was never fully sent, e.g. the request failed before writing started"), and it hangs the exact progress pattern the new UploadTracker docstring recommends — a report_progress() task looping on while not tracker.upload_complete.done() spins forever instead of exiting.

Verified on the branch (AIOHTTP_NO_EXTENSIONS=1, pure Python):

raised: ValueError data and json parameters can not be used at the same time
future done? False attempts 0
raised: TypeError            # non-serializable json= payload
future2 done? False

RuntimeError("Session is closed") and a non-serializable json= object are both reachable at runtime, not just programmer typos.

Fix: move upload_tracker._bind() up to immediately after the self.closed check, and pull the remaining validation inside the try: block that already ends with upload_tracker._finalize(e) — or add a small try/except BaseException around the validation that finalizes and re-raises.

        if upload_tracker is not None:
            upload_tracker._bind()

🟢 Suggestions

2. "Future exception was never retrieved" fires on the documented polling pattern
aiohttp/client_reqrep.py:338-345

The _exc is _final_exc guard only consumes the exception when the request re-raised the same object. When the server answers successfully and the upload fails afterwards (_final_exc is None), the exception is set but never consumed.

A user following the documented example — while not tracker.upload_complete.done(): ... — never calls .exception(), so asyncio logs a scary traceback at GC even though nothing was actually lost. Reproduced on the branch:

Future exception was never retrieved
future: <Future finished exception=ClientConnectionError("Failed to send bytes ...: ValueError('boom')")>

Options: always call fut.exception() after set_exception() (the tracker keeps _exc for anyone who wants it), or make the docs example retrieve the result (await tracker.upload_complete inside a try) so the pattern you advertise is the one that doesn't log.

            fut.set_exception(self._exc)
            if self._exc is self._final_exc:
                fut.exception()

3. Undeclared (and welcome) fix: expect100 rejection now closes the connection
aiohttp/client_reqrep.py:1666-1671

Moving the 100-continue preamble inside the try means a cancellation while awaiting self._continue now hits conn.close(), which it did not before. That is a real behaviour change — and it fixes a pre-existing bug.

On master, when a server answers a expect100=True request with a final status instead of 100 Continue, _response_eof() → _cleanup_writer() cancels the writer mid-preamble and the connection goes back to the pool half-written. Three sequential POSTs over TCPConnector(limit=1, force_close=False):

• master: 2 server transports, request API changed to create_server #1 fails with 400 Bad HTTP method in status line 'P/1.1'
• this branch: 3 server transports, all three return 417 correctly

Since it is an incidental fix in a feature PR, it has no changelog fragment and no test guarding it — easy to silently revert during a future refactor of _write_bytes. Worth a CHANGES/13579.bugfix.rst and a regression test asserting the connection is not reused after an expect100 rejection.

        except asyncio.CancelledError:
            # Body hasn't been fully sent, so the connection can't be reused.
            conn.close()

4. Deprecated attributes removed from the docs instead of marked deprecated
docs/client_reference.rst:1573-1574

ClientResponse.output_size and ClientResponse.upload_complete still exist and still work — they just warn. Their reference entries were deleted outright, so a user who hits the DeprecationWarning has no documentation for the attribute at all until 4.0 removes it.

The repo convention elsewhere in this same file is to keep the entry and annotate it (ssl_shutdown_timeout at line 275, ClientSession.cookie_jar at 309, Fingerprint digests at 816). Suggest restoring both entries with a .. deprecated:: 3.14.4 Use :class:UploadTracker instead. directive, which also lets the CHANGES.rst entry keep its :attr: cross-references rather than being downgraded to literal text.

5. Cancelling `upload_complete` is silently swallowed inside a TaskGroup
aiohttp/client_reqrep.py:346-348

Using fut.cancel() to mean "the body was never sent" makes await tracker.upload_complete raise CancelledError in code that was not cancelled.

Inside asyncio.TaskGroup, a child task that ends in CancelledError is treated as cancellation rather than failure, so a middleware short-circuit or a pre-write connect error would be silently swallowed instead of surfacing. except Exception around the await also won't catch it on 3.8+.

This is an API-shape question rather than a bug — the alternative is a dedicated sentinel exception (e.g. UploadNotSent) or resolving with a status value. Worth deciding now, since the public contract is hard to change after release.

        else:
            # PENDING (the body was never sent) or CANCELLED.
            fut.cancel()

---

Checklist

• New public API is documented — suggestion ＃4
• Changelog fragments present and correctly attributed — suggestion ＃3
• New behaviour is covered by tests — suggestion ＃3
• Error paths settle all observable state — warning ＃1
• No resource leaks (timer handles, writer tasks, connections)
• No backward-incompatible change to existing public API
• Concurrency: stale-attempt events cannot corrupt counters
• No hardcoded secrets or unsafe operations
• Relevant test suites pass locally

---

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

🟠 **6. HIGH** — swallowed exception
aiohttp/client_reqrep.py:1630-1660

Risk: Neither inner handler re-raises, so _write_bytes returns normally after a failed body write — the writer task is reported as successful and the only remaining signals are set_exception(protocol, ...) (a no-op if the protocol already holds an exception or the response was already fully read) and the optional tracker future, so a truncated upload can surface as a clean 200.

            except OSError as underlying_exc:
                ...
                set_exception(protocol, reraised_exc, underlying_exc)
                if tracker is not None:
                    tracker._attempt_failed(gen, reraised_exc)
            except Exception as underlying_exc:
                ...
                set_exception(protocol, wrapped_exc, underlying_exc)
                if tracker is not None:
                    tracker._attempt_failed(gen, wrapped_exc)

Fix: Re-raise the wrapped exception (or record a hard failure flag the response path must check) instead of relying solely on set_exception to carry the error out of band.

🟠 **7. HIGH** — error reported to observer but not to connection
aiohttp/client_reqrep.py:1668-1676

Risk: This new handler covers exactly the paths the inner handlers miss (send_headers/drain/await self._continue/write_eof) but only notifies the tracker — set_exception(protocol, ...) and conn.close() are never called, so the connection is left believing the body is still in flight and the raised exception is discarded entirely when _terminate() drops the writer task instead of _close() awaiting it.

        except BaseException as underlying_exc:
            # Failures escaping the inner handlers (100-continue preamble,
            # write_eof) must still be reported to the tracker.
            if tracker is not None:
                tracker._attempt_failed(gen, underlying_exc)
            raise

Fix: Mirror the inner handlers: call set_exception(protocol, ClientConnectionError(...), underlying_exc) (and close the connection) before re-raising, so the failure reaches the response reader and not just the optional tracker.

🟡 **8. MEDIUM** — non-atomic state transition leaves future permanently pending
aiohttp/client_reqrep.py:1620-1626

Risk: _attempt_started() runs before the try and conn.close() runs before _attempt_cancelled(), so if the on_body_write assignment or conn.close() raises, the tracker stays in ACTIVE forever and _finalize() deliberately defers settling to the attempt — leaving await tracker.upload_complete hanging with no error at all.

        gen = 0 if tracker is None else tracker._attempt_started()
        if tracker is not None:
            writer.on_body_write = functools.partial(tracker._add_bytes, gen)
        try:
            ...
        except asyncio.CancelledError:
            conn.close()
            if tracker is not None:
                tracker._attempt_cancelled(gen)

Fix: Move _attempt_started() inside the try and settle the attempt from a finally block (or notify the tracker before invoking conn.close()) so the tracker always reaches a terminal state.

🟡 **9. MEDIUM** — failure reported only on an optional channel
aiohttp/client_reqrep.py:337-352

Risk: When the server answers before the body finishes (the test_upload_tracker_error_after_response case) the upload error is not the request's exception, so the request returns a successful response and the failure exists only on upload_complete — a future the documented polling pattern (while not ...done()) never awaits, reducing a real upload failure to an asyncio "exception was never retrieved" GC log.

        elif self._state is _UploadState.FAILED:
            assert self._exc is not None
            fut.set_exception(self._exc)
            if self._exc is self._final_exc:
                fut.exception()

Fix: Expose a synchronously readable terminal state (e.g. a tracker.exception attribute or a failed flag) so .done()-style consumers can detect the failure without awaiting the future.

🟡 **10. MEDIUM** — error signalled via CancelledError
aiohttp/client_reqrep.py:350-352

Risk: "The body was never sent" is signalled by cancelling the future, so await tracker.upload_complete raises asyncio.CancelledError — indistinguishable from the caller's own task being cancelled and routinely swallowed by except asyncio.CancelledError / gather / shield handling, and on 3.11+ it also desynchronises Task.cancelling() bookkeeping.

        else:
            # PENDING (the body was never sent) or CANCELLED.
            fut.cancel()

Fix: Settle with a dedicated exception (e.g. a ClientError/UploadNotSentError subclass) rather than fut.cancel(), reserving cancellation for genuine cancellation.

🟡 **11. MEDIUM** — incomplete cleanup on error path
aiohttp/client.py:608-620

Risk: This new setup-failure handler cleans the timer and tracker but never emits trace.send_request_exception(...) for trace configs whose send_request_start already fired (a partially-run trace loop leaves observers with a start and no terminal event) and never closes the data/json payload built just above the try, silently leaking any file handle it holds.

        except BaseException as e:
            tm.close()
            if handle is not None:
                handle.cancel()
            if upload_tracker is not None:
                upload_tracker._finalize(e)
            raise

Fix: Fire send_request_exception for the traces that were already started and close the constructed payload in this handler, matching the cleanup done by the outer except BaseException.

🟡 **12. MEDIUM** — silent truncation masked in progress reporting
aiohttp/http_writer.py:203-216

Risk: A body producing more data than the declared Content-Length is silently truncated with no error, and bytes_written is now deliberately clamped to the accepted prefix, so an observer comparing bytes_written against the expected payload size sees a self-consistent "complete" upload instead of the discrepancy that would reveal the truncation.

            else:
                # Bytes clipped at the declared length are discarded, not
                # sent: report only the accepted prefix ...
                body_size = self.length
                chunk = chunk[: self.length]
                self.length = 0

Fix: Raise (or at minimum record a truncation flag on the writer/tracker) when a chunk exceeds the remaining declared length, rather than discarding the excess and reporting the clipped count as the accepted size.

---

Automated review by Kōan (Claude) HEAD=74236ef 10 min 35s

[Dreamsorcerer]

Swallowed exception

Verdict: no change — the swallow is load-bearing, and every dangerous reading of this finding is blocked by a verified guarantee. Sub-claim by sub-claim:

"Neither inner handler re-raises" — deliberate routing, not an accident, and it's master's exact behavior (my restructure preserved the arms verbatim). The error is re-raised in the case that matters: when the
response hasn't arrived, set_exception(protocol, ...) makes resp.start() raise it out of _request — the same object, pinned by test_upload_tracker_upload_error_propagated_to_caller (identity assert). If the arms
also re-raised from the writer task, every early-response failure would additionally produce an unawaited writer-task exception → Task exception was never retrieved GC noise on a path that already delivered its
error. The swallow exists to prevent exactly the class of spam we just spent a round eliminating.

"set_exception is a no-op if the protocol already holds an exception" — the delivery may be, but the hygiene isn't: ResponseHandler.set_exception sets _should_close = True (client_proto.py:209), and should_close
is independently true whenever _exception is not None (client_proto.py:80-85). First poison wins; the flag stands either way.

"or the response was already fully read" — then the poisoned protocol delivers to no reader, but should_close still forces the connector to discard the connection on release. A truncated upload can never leak a
dirty connection back into the pool. That's the difference between an observability gap and a correctness bug; this is only the former.

"a truncated upload can surface as a clean 200" — the 200 is the server's genuine verdict, emitted before or while the write died; aiohttp fabricates nothing. Early-response-then-abandoned-body is a legitimate
pattern (413, auth challenges, servers that don't want the tail), and the request succeeding there is documented contract — the proposal's own "request can still succeed if the server responded before reading the
whole body". The signal channel for callers who care is precisely the tracker: _attempt_failed records directly on it, bypassing the protocol entirely, so the "only remaining signals" caveat never degrades it —
pinned by test_upload_tracker_error_after_response (200 + ClientConnectionError on the future).

Making truncation unconditionally fatal on an otherwise-successful request (raise at release/ctx-exit when the writer failed) would be a breaking semantic change to long-standing client behavior and belongs to its
own design discussion, not this PR — say the word if you want that argued separately. As implemented: no-tracker callers keep master's semantics exactly; tracker callers get the previously-nonexistent signal.

[Dreamsorcerer]

@aiolibsbot revewe

[aiolibsbot]

❌ Unknown command revewe. Did you mean review?

Code & Development

• 🔎 @aiolibsbot audit — Audit a project for optimizations, simplifications, and issues — creates tracker issues for findings
• 🧠 @aiolibsbot brainstorm — Break down a topic into detailed sub-issues grouped under a master issue
• 🔎 @aiolibsbot check_need (alias: need, needs) — "Analyze whether a PR's changes or an issue's request is still relevant"
• \U0001f41b @aiolibsbot debug (alias: dbg) — "Run a structured debug loop on a failed issue"
• 🧠 @aiolibsbot deepplan (alias: deeplan) — Deep design an idea — explores approaches, posts spec as GitHub issue, queues /plan
• 📚 @aiolibsbot doc (alias: docs) — Investigate a project codebase and produce structured documentation under docs/
• 💡 @aiolibsbot explain (alias: xp) — "Explain a PR's changes in simple words with examples and alternative approaches"
• 🐞 @aiolibsbot fix — "Queue a fix mission for a GitHub or Jira issue — understand, plan, test, implement, and submit a PR. Can also batch-queue all open GitHub issues from a repo URL. Use --now to queue at the top."
• 🔨 @aiolibsbot implement (alias: impl) — "Queue an implementation mission for a GitHub or Jira issue"
• 🧠 @aiolibsbot plan — Plan an idea or iterate on an existing tracker issue
• 🧠🔨 @aiolibsbot planimplement (alias: planimp, planimpl, planit, plandoit, doit) — "Queue /plan then /implement for an issue — plan insights feed the implementation"
• 📊 @aiolibsbot profile (alias: perf, benchmark) — Queue a performance profiling mission
• 🛠️ @aiolibsbot refactor (alias: rf) — "Queue a refactoring mission for a PR, issue, or file"
• 🔍 @aiolibsbot review (alias: rv, rereview, re_review) — "Queue a code review for one or more PRs/issues. Use --now to queue at the top. Flags: --architecture (SOLID/layering focus), --errors (silent-failure-hunter pass), --comments (comment quality), --plan-url (plan alignment check), --force (review even if closed/merged)"
• 🛡️ @aiolibsbot security_audit (alias: security, secu) — SDLC security audit — finds critical vulnerabilities and creates tracker issues for each
• 🏭 @aiolibsbot speckit — "Run the spec-kit SDD pipeline for a project goal or tracker issue"
• 🌿 @aiolibsbot speckit_from_branch — "Run spec-kit plan onward from a human-authored spec already pushed to a branch"
• 🔬 @aiolibsbot ultrareview (alias: urv, ultra_review) — "Queue the most thorough review Kōan can run: architecture-focused main pass + silent-failure-hunter pass in a single comment. Use --now to queue at the top."

Pull Requests

• ❓ @aiolibsbot ask (alias: question) — "Ask a question about a PR or issue and get an AI reply posted to GitHub"
• 🔀 @aiolibsbot gh_request — "Route a natural-language GitHub request to the appropriate action (fix, rebase, review, reply, etc.)"
• 🔄 @aiolibsbot rebase (alias: rb) — "Queue a PR rebase (ex: /rebase https://github.com/owner/repo/pull/42). Bare rebase = rebase only; add --fix to also apply review feedback. Use --now to queue at the top."
• 🔁 @aiolibsbot recreate (alias: rc) — "Recreate a diverged PR from scratch on current upstream (ex: /recreate https://github.com/owner/repo/pull/42)"
• 🔍 @aiolibsbot reviewrebase (alias: rr) — "Queue /review then /rebase --fix for a PR — review insights feed the rebase, which then applies the feedback"
• 🔄 @aiolibsbot squash (alias: sq) — "Squash PR commits into one (ex: /squash https://github.com/owner/repo/pull/42)"

Usage: @aiolibsbot <command> in any PR or issue comment.