Skip to content

stability: multipart fetch pool #12205

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
azr wants to merge 1 commit into
base: main
Choose a base branch
from
Open

stability: multipart fetch pool #12205

azr wants to merge 1 commit into from

Conversation

@azr
Copy link
Contributor

@azr azr commented Aug 14, 2025 •
edited
Loading

this:

  • makes sure we reset a pooled buffer when releasing it while multipart fetch is on
  • fixes a potential goroutine leak when we cancel a request
  • closes the chunk writer if we couldn't acquire a lock <- this could potentially lock if a reader was trying to read, I think

@github-project-automation github-project-automation bot moved this to Needs Triage in Pull Request Review Aug 14, 2025
@k8s-ci-robot
Copy link

k8s-ci-robot commented Aug 14, 2025

Hi @azr. Thanks for your PR.

I'm waiting for a containerd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@dosubot dosubot bot added area/distribution Image Distribution kind/bug labels Aug 14, 2025
@azr azr force-pushed the multipart-fetch-stability branch from 50ee2e3 to bdcc07a Compare August 14, 2025 12:41
@azr azr changed the title multipart fetch stability fixes fix: multipart fetch pool Aug 20, 2025
@azr azr force-pushed the multipart-fetch-stability branch from bdcc07a to 1f31369 Compare August 20, 2025 09:01
@azr azr marked this pull request as draft August 22, 2025 06:44
@azr azr marked this pull request as ready for review August 26, 2025 15:13
fuweid
fuweid reviewed Aug 27, 2025
View reviewed changes
@azr azr force-pushed the multipart-fetch-stability branch 2 times, most recently from ae2a666 to 36d4544 Compare August 29, 2025 15:26
djdongjin
djdongjin reviewed Aug 30, 2025
View reviewed changes
@azr azr force-pushed the multipart-fetch-stability branch 2 times, most recently from 15c25cf to 04ffc4b Compare September 4, 2025 06:50
@k8s-ci-robot k8s-ci-robot added size/M and removed size/S labels Sep 4, 2025
@azr azr force-pushed the multipart-fetch-stability branch 3 times, most recently from 227b04e to a4ab73e Compare September 8, 2025 13:29
@azr azr changed the title fix: multipart fetch pool stability: multipart fetch pool Sep 15, 2025
@azr azr requested review from djdongjin and fuweid September 15, 2025 08:00
fuweid
fuweid reviewed Sep 16, 2025
View reviewed changes
core/remotes/docker/fetcher.go
body = &fnOnClose{
BeforeClose: func() {
cancelCtx()
if err := eg.Wait(); err != nil {
Copy link
Member

@fuweid fuweid Sep 16, 2025

Choose a reason for hiding this comment

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

I think we still need cancelctx call here and then wait.

Copy link
Contributor Author

@azr azr Sep 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

cancelCtx does not exist anymore in this case. the ctx will be cancelled when all eg (error group) goroutines have returned. ( and if any return with an error eg will return without waiting for the others )

Copy link
Member

@fuweid fuweid Jan 21, 2026

Choose a reason for hiding this comment

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

If there is error returned by Line 595, BeforeClose will be closed in defer. But there is no cancel event, right? the close will be waiting until all the goroutines finish. Is it correct?

Copy link
Contributor Author

@azr azr Jan 21, 2026

Choose a reason for hiding this comment

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

ha this is a very very good point, added a cancel.

@azr azr requested a review from fuweid September 24, 2025 13:51
@azr azr force-pushed the multipart-fetch-stability branch 2 times, most recently from 977a93a to cca6ba5 Compare October 2, 2025 06:38
henry118
henry118 reviewed Oct 17, 2025
View reviewed changes
@azr azr force-pushed the multipart-fetch-stability branch 3 times, most recently from 637bc96 to 706ab62 Compare November 10, 2025 14:29
@djdongjin djdongjin requested a review from Copilot November 12, 2025 02:52
Copilot AI reviewed Nov 12, 2025
View reviewed changes
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR enhances the stability of the multipart fetch pool implementation by addressing potential resource leaks and improving error handling in parallel download scenarios.

Key changes:

  • Resets buffers before returning them to the pool to prevent data leakage across requests
  • Replaces manual goroutine management with errgroup for proper lifecycle handling and error propagation
  • Introduces context-aware copying to respect cancellation during data transfers

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

@azr azr force-pushed the multipart-fetch-stability branch from 706ab62 to 340ae3b Compare November 27, 2025 07:52
@azr azr force-pushed the multipart-fetch-stability branch 3 times, most recently from 15bca23 to 4024987 Compare December 11, 2025 11:15
@azr
Copy link
Contributor Author

azr commented Dec 11, 2025

@hsiangkao @henry118 @djdongjin @fuweid can I get another review round on this?🙏

Updated — switched to errgroup, added context-aware copy, fixed buffer pool reset.

@azr azr mentioned this pull request Dec 15, 2025
Closed
@azr azr force-pushed the multipart-fetch-stability branch from 4024987 to bea67d2 Compare December 18, 2025 07:50
henry118
henry118 approved these changes Jan 6, 2026
View reviewed changes
Copy link
Member

@henry118 henry118 left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks

@github-project-automation github-project-automation bot moved this from Needs Triage to Review In Progress in Pull Request Review Jan 6, 2026
@azr
multipart fetch stability fixes
e69a33a 
Signed-off-by: Adrien Delorme <azr@users.noreply.github.com>
@azr azr force-pushed the multipart-fetch-stability branch from bea67d2 to e69a33a Compare January 21, 2026 21:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

@henry118 henry118 henry118 approved these changes

@djdongjin djdongjin djdongjin approved these changes

@fuweid fuweid Awaiting requested review from fuweid

At least 2 approving reviews are required to merge this pull request.

Assignees

No one assigned

Labels

area/distribution Image Distribution kind/bug needs-ok-to-test size/M

Projects

Pull Request Review
Status: Review In Progress

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@azr @k8s-ci-robot @henry118 @fuweid @djdongjin