Repository navigation
fix(slack-bridge): stream file downloads to disk instead of buffering - #2172
Conversation
Same 1-line upgrade #1886 applied to telegram-bridge: the Slack file download buffered the whole response in memory via resp.read(); use shutil.copyfileobj so a large shared file can't balloon bridge memory. Timeout and auth behavior unchanged. Stand: Echo Act IV Pro Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014WZ5hTbeXUq1zQkTTsqA5H
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 63%. Diff CoverageDiff: origin/main...HEAD, staged and unstaged changes
Summary
|
…ate) CountingResponse serves a 1 MiB payload and counts read() calls: the streamed implementation takes many bounded reads; the pre-fix buffered shape took exactly one unbounded read. Also asserts payload integrity, the auth header, and the 30s timeout. Stand: Echo Act IV Pro Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014WZ5hTbeXUq1zQkTTsqA5H
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
Approve — memory-safety fix done right. Streaming Slack file downloads to disk via shutil.copyfileobj(resp, f) in bounded chunks replaces f.write(resp.read()) (whole-file-into-RAM → OOM risk on big attachments). The test is genuinely behavioral, not a source-grep: it stubs urlopen with a read-counting response and asserts multiple bounded reads occur (the pre-fix shape produced one unbounded read) + round-trips the bytes for content integrity. Single concern, CI green (10/10). Bot approval — needs a 2nd human per the ruleset.
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
Reviewed and safe to merge (owner-directed PR triage).
Source change is a one-liner: f.write(resp.read()) → shutil.copyfileobj(resp, f) (+ import shutil). Streams the download to disk in bounded chunks instead of buffering the entire file in memory — strictly better, same output bytes.
No live test necessary here (unlike a behavioral bridge-flow change): the unit test is a real behavioral proof, not a source grep — it stubs urlopen with a 1 MiB payload and asserts copyfileobj takes multiple bounded reads (where the old resp.read() took exactly one unbounded read), plus round-trips the bytes for content integrity. copyfileobj is stdlib; regression risk is minimal. CI green.
LGTM. (Bot approval = 1 of 2; needs a maintainer sign-off + owner merge.)
|
Reviewed, LGTM: shutil.copyfileobj is the right one-line fix, and the test is a real behavioral proof — CountingResponse asserting read_calls > 1 AND max_read_size < payload pins streaming, not just a source grep. Auth header + timeout preservation checks are a nice touch. — @sutando-qingyun-001 |
liususan091219
left a comment
liususan091219
left a comment
There was a problem hiding this comment.
Justification: [change-requested: missing_before_after]
- missing_before_after: Add before/after download evidence showing bounded reads or memory drop.
Retracted: this change-request was auto-posted by a triage sweep that has been stopped per the maintainer. Dismissing so it no longer blocks the PR.
…-main # Conflicts: # src/slack-bridge.py
|
@cla-assistant check |
Follow-up promised in #1886 (comment): slack-bridge's file download buffers the entire response in memory (
f.write(resp.read())). This applies the same 1-line streaming upgrade #1886 f2f16f5 gave telegram-bridge —shutil.copyfileobj(resp, f)at the same call site. Timeout (30s) and auth header unchanged. Single concern; no behavior change beyond memory profile.Stand: Echo Act IV Pro
🤖 Generated with Claude Code
https://claude.ai/code/session_014WZ5hTbeXUq1zQkTTsqA5H