Skip to content

s3/transfermanager: avoid double-closing concurrentReader channel after read errors - #3389

Merged
wty-Bryant merged 4 commits into
aws:mainfrom
islishude:fix-s3-tm-double-close-channel
Jun 5, 2026
Merged

s3/transfermanager: avoid double-closing concurrentReader channel after read errors#3389
wty-Bryant merged 4 commits into
aws:mainfrom
islishude:fix-s3-tm-double-close-channel

Conversation

@islishude

Copy link
Copy Markdown
Contributor

Summary

Fix a panic in transfermanager concurrentReader when Read is called again after a non-EOF error.

Problem

When concurrentReader.Read encounters a non-EOF error, it closes r.ch to unblock the internal reader goroutine. If the caller invokes Read again, the method previously continued through the same cleanup path and attempted to close r.ch a second time, causing this panic: panic: close of closed channel

Changes

Return the stored non-EOF error immediately on subsequent Read calls
Guard r.ch close with sync.Once so channel shutdown is idempotent
Add a regression test covering repeated Read calls after a download error

Test

go test ./... in feature/s3/transfermanager

Copilot AI review requested due to automatic review settings April 16, 2026 12:08
@islishude
islishude requested a review from a team as a code owner April 16, 2026 12:08

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes a panic in transfermanager’s concurrentReader when Read is invoked again after a previous non-EOF read error, by making channel shutdown and error re-reads idempotent.

Changes:

  • Return the stored non-EOF error immediately on subsequent Read calls.
  • Guard r.ch shutdown with sync.Once to prevent double-close panics.
  • Add a regression test for repeated Read calls after a download error.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
feature/s3/transfermanager/concurrent_reader.go Prevents double-closing r.ch and short-circuits repeated reads after non-EOF errors.
feature/s3/transfermanager/concurrent_reader_test.go Adds a regression test covering repeated Read calls after an injected range-download error.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread feature/s3/transfermanager/concurrent_reader_test.go Outdated
@islishude

Copy link
Copy Markdown
Contributor Author

Hi there, could you team please take a look? and please also #3388

@wty-Bryant

Copy link
Copy Markdown
Contributor

sorry, dealing with some other feature parity this week, will look into those 2 PRs recently

Comment thread feature/s3/transfermanager/concurrent_reader.go Outdated
@wty-Bryant
wty-Bryant force-pushed the fix-s3-tm-double-close-channel branch 2 times, most recently from f2118be to 269e116 Compare June 5, 2026 16:32
@wty-Bryant
wty-Bryant force-pushed the fix-s3-tm-double-close-channel branch from 269e116 to c3ae025 Compare June 5, 2026 18:55
@wty-Bryant
wty-Bryant merged commit 51692f8 into aws:main Jun 5, 2026
11 of 13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants