-
Notifications
You must be signed in to change notification settings - Fork 233
feat: add command for optimizing existing builds #1709
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.
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
base: main
Are you sure you want to change the base?
Conversation
| return fmt.Errorf("failed to create sandbox proxy: %w", err) | ||
| } | ||
| go func() { | ||
| err := sandboxProxy.Start(parentCtx) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The goroutine at line 144 uses parentCtx instead of ctx for starting sandboxProxy. This means sandboxProxy.Start() ignores the 10-minute operation timeout and could run indefinitely even if the optimization fails or times out. Consider using ctx instead for proper timeout propagation.
| noop.NewMeterProvider(), | ||
| ) | ||
| go func() { | ||
| err := tcpFirewall.Start(ctx) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tcpFirewall.Start() goroutine uses ctx (the timeout context) but tcpFirewall.Close() in the defer uses parentCtx. This creates inconsistency - if the timeout fires and cancels ctx, the Start goroutine will stop, but the Close() will use a different context. Consider using the same context for both operations.
| return fmt.Errorf("could not create device pool: %w", err) | ||
| } | ||
| go func() { | ||
| devicePool.Populate(ctx) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The devicePool.Populate() and networkPool.Populate() goroutines use ctx (timeout context). If the optimization completes or times out before these pools finish populating, the goroutines will be canceled but may still be running during cleanup. This could cause resource leaks or race conditions. Consider waiting for these goroutines to complete or using an errgroup to manage them properly.
| ) | ||
|
|
||
| // Create build context | ||
| uploadErrGroup := &errgroup.Group{} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The uploadErrGroup created at line 245 is never waited on with .Wait(). If any upload operations are scheduled via this errgroup during optimization, their errors will be silently ignored and goroutines may be leaked when the function returns. Either add a defer with uploadErrGroup.Wait() or document that no uploads are expected during optimization.
f342294 to
68db802
Compare
99db1f3 to
bde27e0
Compare
bde27e0 to
a01add7
Compare
No description provided.