KUBEDR-7763: Don't panic if CanShareWork() is called on a closed workpool#442
Merged
dzaninovic merged 1 commit intov0.19.0.xfrom Apr 17, 2026
Merged
KUBEDR-7763: Don't panic if CanShareWork() is called on a closed workpool#442dzaninovic merged 1 commit intov0.19.0.xfrom
dzaninovic merged 1 commit intov0.19.0.xfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the internal/workshare worker pool to avoid panicking when AsyncGroup.CanShareWork() is called after the pool has been closed, and instead emits a warning log (requiring context to be passed into pool construction so logging can use the correct context-derived log level behavior).
Changes:
- Change
workshare.NewPoolto accept acontext.Contextand store it on the pool. - Update
AsyncGroup.CanShareWork()to returnfalse(and warn) instead of panicking when the pool is closed. - Update snapshotfs call sites and workshare tests to pass context into
NewPool, and adjust expectations for the newCanShareWork()behavior.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| snapshot/snapshotfs/upload.go | Passes ctx into workshare.NewPool when creating the uploader worker pool. |
| snapshot/snapshotfs/snapshot_tree_walker.go | Passes ctx into workshare.NewPool for TreeWalker parallel traversal. |
| snapshot/snapshotfs/dir_rewriter.go | Passes ctx into workshare.NewPool for directory rewriting work sharing. |
| internal/workshare/workshare_waitgroup.go | Replaces CanShareWork() panic-on-closed with warning + false return. |
| internal/workshare/workshare_test.go | Updates tests/benchmarks to use the new NewPool(ctx, n) signature and new CanShareWork() behavior. |
| internal/workshare/workshare_pool.go | Adds context storage to Pool and updates NewPool signature accordingly. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
draghuram
approved these changes
Apr 17, 2026
Collaborator
Author
|
Kopia linter did not like me passing context in a struct. I will fix that. |
Collaborator
Author
|
We will fix the linter issue later. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Don't panic if CanShareWork() is called on a closed workpool
Log a warning instead.
Context passing is needed to make the log levels work.
I tested to make sure the log is printed in our mover log.