Skip to content

Add join handle for StartDockerImageDownload to bound goroutine lifecycle - #47974

Merged
pelikhan merged 6 commits into
mainfrom
copilot/deep-report-add-join-point-to-startdockerimagepull
Jul 25, 2026
Merged

Add join handle for StartDockerImageDownload to bound goroutine lifecycle#47974
pelikhan merged 6 commits into
mainfrom
copilot/deep-report-add-join-point-to-startdockerimagepull

Conversation

Copilot AI commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

StartDockerImageDownload previously launched an unjoinable background goroutine, so callers had no way to observe completion and cleanup could run after caller scope moved on. This change introduces an explicit join point and tracks per-image in-flight workers to make lifecycle and state mutation bounded.

  • Concurrency contract update

    • Change StartDockerImageDownload signature to return (started bool, join func()).
    • join() blocks until the image’s background pull goroutine exits.
  • Per-image in-flight join tracking

    • Add inflight map[string]chan struct{} alongside downloading.
    • Store a done channel when a pull starts; close it on worker exit.
    • If a second caller hits an already-downloading image, return started=false plus a join function for the existing worker.
  • State cleanup hardening

    • On worker exit, remove both downloading[image] and inflight[image] under lock.
    • For already-available images, return a no-op join function to keep call sites uniform.
  • Call-site and behavior updates

    • Update internal call sites to handle the new return shape.
    • Extend docker image tests to cover:
      • joining an existing in-flight download
      • no-op join behavior when no worker is started
started, join := StartDockerImageDownload(ctx, image)
if started {
    // optional: continue work while pull runs
}
join() // deterministic synchronization point for cleanup/completion

Run: https://github.com/github/gh-aw/actions/runs/30156126450

Generated by 👨‍🍳 PR Sous Chef · gpt54 · 2.97 AIC · ⌖ 8.03 AIC · ⊞ 7.1K ·
Comment /souschef to run again

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Copilot AI changed the title [WIP] Add join point to StartDockerImagePull goroutine Add join handle for StartDockerImageDownload to bound goroutine lifecycle Jul 25, 2026
Copilot AI requested a review from pelikhan July 25, 2026 10:57
@pelikhan
pelikhan marked this pull request as ready for review July 25, 2026 11:00
Copilot AI review requested due to automatic review settings July 25, 2026 11:00

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Adds join handles to Docker image downloads for deterministic worker synchronization and cleanup.

Changes:

  • Tracks per-image in-flight workers.
  • Returns join functions from download starts.
  • Adds join and cancellation tests.
Show a summary per file
File Description
pkg/cli/docker_images.go Adds worker tracking and join handles.
pkg/cli/docker_images_test.go Updates and extends concurrency tests.
pkg/cli/docker_images_test_helpers_test.go Resets in-flight test state.

Review details

Tip

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

  • Files reviewed: 3/3 changed files
  • Comments generated: 2
  • Review effort level: Medium

Comment thread pkg/cli/docker_images.go Outdated
Comment on lines +143 to +146
// Check if already available (inside lock for atomicity)
if isDockerImageAvailableUnlocked(ctx, image) {
dockerImagesLog.Printf("Image %s is already available", image)
return false
return false, func() {}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in e0c8d4e by checking downloading before image availability, so callers now keep observing the in-flight worker until its cleanup finishes.

Comment on lines +538 to +540
cancel()
joinFirst()
joinSecond()

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in e0c8d4e by making joinSecond() run asynchronously, asserting it stays blocked while the shared worker is active, then canceling and verifying it completes.

@pelikhan

Copy link
Copy Markdown
Collaborator

@copilot run pr-finisher skill

@pelikhan

Copy link
Copy Markdown
Collaborator

@copilot run pr-finisher skill

@gh-aw-bot

Copy link
Copy Markdown
Collaborator

@copilot please run the pr-finisher skill, address the unresolved review threads, and rerun checks once the branch is up to date.

Unresolved review threads:

Generated by 👨‍🍳 PR Sous Chef · gpt54 · 2.97 AIC · ⌖ 8.03 AIC · ⊞ 7.1K ·
Comment /souschef to run again

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>

Copilot AI commented Jul 25, 2026

Copy link
Copy Markdown
Contributor Author

@copilot run pr-finisher skill

Ran the PR-finisher pass. Addressed the two open review threads in e0c8d4e and revalidated locally.

Copilot AI commented Jul 25, 2026

Copy link
Copy Markdown
Contributor Author

@copilot run pr-finisher skill

Ran the PR-finisher pass on 1495b0c. The branch is up to date with origin/main, and I reran the local checks on the latest head.

@copilot please run the pr-finisher skill, address the unresolved review threads, and rerun checks once the branch is up to date.

Unresolved review threads:...

Ran the PR-finisher pass on 1495b0c. The branch is already up to date with origin/main, the two review comments are already addressed in that commit, and make fmt, make lint, TEST_UNIT_RUN_FULL=1 make test-unit, make test, and make recompile all passed locally. I can’t resolve the remaining review threads from this environment because gh auth is unavailable here.

Copilot AI requested a review from gh-aw-bot July 25, 2026 11:42
@pelikhan
pelikhan merged commit 545bc58 into main Jul 25, 2026
24 checks passed
@pelikhan
pelikhan deleted the copilot/deep-report-add-join-point-to-startdockerimagepull branch July 25, 2026 11:50
@github-actions

Copy link
Copy Markdown
Contributor

🎉 This pull request is included in a new release.

Release: v0.83.4

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.

[deep-report] Add a join point to StartDockerImagePull goroutine (docker_images.go:155)

4 participants