Stop file transfers when HTTP clients disconnect - #3523
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Docs preview: https://d3d4d8c6-starlette.marcelotryle.workers.dev |
Merging this PR will improve performance by 16.23%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ⚡ | Simulation | test_multipart[boundary-like-file] |
9.2 ms | 7.9 ms | +16.23% |
Tip
Curious why performance improved? Comment @codspeedbot explain why performance improved on this PR, or directly use the CodSpeed MCP with your agent.
Comparing codex/file-response-disconnect (0a781cb) with main (3ebcaf3)
| @asynccontextmanager | ||
| async def _open_file(self) -> AsyncIterator[anyio.AsyncFile[bytes]]: | ||
| file = await anyio.open_file(self.path, mode="rb") | ||
| try: | ||
| yield file | ||
| finally: | ||
| # Closing must finish even when the transfer is cancelled. | ||
| with anyio.CancelScope(shield=True): | ||
| await file.aclose() |
There was a problem hiding this comment.
@agronholm Isn't there something builtin in anyio that I can use instead of my own wrapper?
There was a problem hiding this comment.
This particular issue was fixed in AnyIO v4.15.0.
There was a problem hiding this comment.
Are you sure? My coding agent seems to think only TemporaryDirectory was fixed. 🤔
There was a problem hiding this comment.
You're right. Somehow I was under the impression that it was a broader fix for open files, but it's not. The same issue is present in AsyncFile too. I'll get this fixed for the next patch release.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
2 issues found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="benchmarks/file_response_benchmark.py">
<violation number="1">
P3: This change removes the `spec_version` parametrization (`["2.3", "2.5"]`), so the file-response benchmarks now only measure the ASGI 2.5 direct path. The ASGI <2.4 disconnect-listener path added by this PR (a concurrent task group with a blocking `receive()` listener per request) is no longer benchmarked, so a regression in that new hot path would go undetected.</violation>
</file>
<file name="tests/test_responses.py">
<violation number="1" location="tests/test_responses.py:463">
P3: The upper bound `submitted < 3 * FileResponse.chunk_size` is a race heuristic: how many chunks are sent before the disconnect listener cancels `stream_file` depends on event-loop scheduling. On a slower scheduler or under Trio's checkpointing, more chunks could be delivered before the cancel lands, making this assertion flaky. The meaningful invariant is that the transfer stops after the disconnect is observed, not a specific chunk count.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| with anyio.fail_after(5): | ||
| await FileResponse(path, background=BackgroundTask(cleanup))(scope, receive, send) | ||
|
|
||
| assert FileResponse.chunk_size <= submitted < 3 * FileResponse.chunk_size |
There was a problem hiding this comment.
P3: The upper bound submitted < 3 * FileResponse.chunk_size is a race heuristic: how many chunks are sent before the disconnect listener cancels stream_file depends on event-loop scheduling. On a slower scheduler or under Trio's checkpointing, more chunks could be delivered before the cancel lands, making this assertion flaky. The meaningful invariant is that the transfer stops after the disconnect is observed, not a specific chunk count.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/test_responses.py, line 463:
<comment>The upper bound `submitted < 3 * FileResponse.chunk_size` is a race heuristic: how many chunks are sent before the disconnect listener cancels `stream_file` depends on event-loop scheduling. On a slower scheduler or under Trio's checkpointing, more chunks could be delivered before the cancel lands, making this assertion flaky. The meaningful invariant is that the transfer stops after the disconnect is observed, not a specific chunk count.</comment>
<file context>
@@ -395,6 +396,130 @@ async def send(message: Message) -> None:
+ with anyio.fail_after(5):
+ await FileResponse(path, background=BackgroundTask(cleanup))(scope, receive, send)
+
+ assert FileResponse.chunk_size <= submitted < 3 * FileResponse.chunk_size
+ assert background_ran
+
</file context>
2 participants
FileResponsecan continue reading an entire file after the client disconnects on ASGI versions before 2.4. Run its existing file handler alongside anhttp.disconnectlistener and shield file closure from cancellation, so aborted full-file and range transfers stop and close the file before background cleanup.Alternative to #3390, following the focused approach in libratechw's proposal: preserve
StreamingResponse, handler signatures, and the direct HEAD, pathsend, WebSocket denial, and ASGI 2.4+ paths.Validation:
scripts/test(1,295 passed, 100% coverage),scripts/build, and the four existing file-response benchmark cases passed; regression tests cover both asyncio and Trio, and fail against both the original implementation and an unshieldedaclosing()variant.AI Disclaimer
This PR was developed with the assistance of either Claude or Codex. I've reviewed and verified the changes.