refactor(agent): move retry logic and binary management to binary.go#445
Merged
refactor(agent): move retry logic and binary management to binary.go#445
Conversation
Signed-off-by: Samuel K <skevetter@pm.me>
Signed-off-by: Samuel K <skevetter@pm.me>
Signed-off-by: Samuel K <skevetter@pm.me>
Signed-off-by: Samuel K <skevetter@pm.me>
Signed-off-by: Samuel K <skevetter@pm.me>
📝 WalkthroughWalkthroughExtracts retry, binary acquisition, caching, and HTTP download logic into Changes
Sequence Diagram(s)sequenceDiagram
participant Manager as BinaryManager
participant Inject as InjectSource
participant Cache as BinaryCache
participant HTTP as HTTPDownloadSource
participant Disk as Filesystem
rect rgba(200,220,255,0.5)
Manager->>Inject: GetBinary(arch)
alt Inject matches runtime
Inject-->>Manager: io.ReadCloser (executable)
else Inject not available
Inject-->>Manager: error
Manager->>Cache: Get(arch)
alt Cache hit
Cache-->>Manager: io.ReadCloser (cached file)
else Cache miss
Cache-->>Manager: error
Manager->>HTTP: GetBinary(arch) -> buildDownloadURL
HTTP->>HTTP: downloadFile(ctx, URL)
HTTP->>Disk: streamAndCache -> atomic write via BinaryCache
HTTP-->>Manager: io.ReadCloser (streaming reader)
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@pkg/agent/binary.go`:
- Around line 217-222: atomicWrite currently returns os.Rename(temp, path)
directly which can leave the temp file on disk if the rename fails; change
atomicWrite so it captures the error from os.Rename and, on non-nil error,
removes the temporary file (using os.Remove(temp)) before returning the error,
referencing the existing temp and path variables and the atomicWrite function to
locate the change.
- Around line 365-371: In streamAndCache, when os.CreateTemp fails the current
fallback ignores the io.Copy error; modify the CreateTemp error path to capture
the result of io.Copy(body, pw) (e.g., errCopy), assign it to *streamErr (so the
caller sees the failure) and close the pipe writer with that error
(pw.CloseWithError(errCopy) or pw.Close() after setting *streamErr) to ensure
the stream error is propagated; reference the streamAndCache function, the pw
pipe writer, the streamErr pointer, and the os.CreateTemp fallback path when
making the change.
Signed-off-by: Samuel K <skevetter@pm.me>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@pkg/agent/binary.go`:
- Around line 142-176: AcquireBinary currently loops all sources and calls
GetBinary without checking ctx, so a canceled/expired context can be bypassed
(notably FileCacheSource ignores ctx) and return a binary after cancellation;
update BinaryManager.AcquireBinary to check ctx before each iteration (select on
ctx.Done()) and immediately return nil, "", ctx.Err() when canceled, and ensure
GetBinary implementations (e.g., FileCacheSource.GetBinary) observe the passed
ctx where blocking I/O or sleeps occur so they can return early on ctx
cancellation.
- Around line 94-111: The deadline check in RetryConfig.checkDeadline currently
treats now==deadline as allowed because it uses !time.Now().After(cfg.Deadline);
change the logic so equality is considered expired by returning nil only when
Deadline is zero or now is strictly before the deadline (use
time.Now().Before(cfg.Deadline) instead of !time.Now().After(...)); update
RetryConfig.checkDeadline to enforce expiration on equality and keep the
returned error using ErrInjectTimeout and the attemptsCompleted value unchanged.
Comment on lines
+94
to
+111
| func (cfg *RetryConfig) applyDefaults() { | ||
| if cfg.MaxAttempts <= 0 { | ||
| cfg.MaxAttempts = 1 | ||
| } | ||
| if cfg.InitialDelay <= 0 { | ||
| cfg.InitialDelay = time.Second | ||
| } | ||
| if cfg.MaxDelay <= 0 { | ||
| cfg.MaxDelay = 30 * time.Second | ||
| } | ||
| } | ||
|
|
||
| func (cfg *RetryConfig) checkDeadline(attemptsCompleted int) error { | ||
| if cfg.Deadline.IsZero() || !time.Now().After(cfg.Deadline) { | ||
| return nil | ||
| } | ||
| return fmt.Errorf("%w after %d attempts", ErrInjectTimeout, attemptsCompleted) | ||
| } |
There was a problem hiding this comment.
Treat deadline equality as expired.
Line 107 uses time.Now().After, which doesn’t trip when now == deadline; that can permit one more attempt/sleep past the deadline.
🛠️ Suggested fix
func (cfg *RetryConfig) checkDeadline(attemptsCompleted int) error {
- if cfg.Deadline.IsZero() || !time.Now().After(cfg.Deadline) {
+ if cfg.Deadline.IsZero() || time.Now().Before(cfg.Deadline) {
return nil
}
return fmt.Errorf("%w after %d attempts", ErrInjectTimeout, attemptsCompleted)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func (cfg *RetryConfig) applyDefaults() { | |
| if cfg.MaxAttempts <= 0 { | |
| cfg.MaxAttempts = 1 | |
| } | |
| if cfg.InitialDelay <= 0 { | |
| cfg.InitialDelay = time.Second | |
| } | |
| if cfg.MaxDelay <= 0 { | |
| cfg.MaxDelay = 30 * time.Second | |
| } | |
| } | |
| func (cfg *RetryConfig) checkDeadline(attemptsCompleted int) error { | |
| if cfg.Deadline.IsZero() || !time.Now().After(cfg.Deadline) { | |
| return nil | |
| } | |
| return fmt.Errorf("%w after %d attempts", ErrInjectTimeout, attemptsCompleted) | |
| } | |
| func (cfg *RetryConfig) checkDeadline(attemptsCompleted int) error { | |
| if cfg.Deadline.IsZero() || time.Now().Before(cfg.Deadline) { | |
| return nil | |
| } | |
| return fmt.Errorf("%w after %d attempts", ErrInjectTimeout, attemptsCompleted) | |
| } |
🤖 Prompt for AI Agents
In `@pkg/agent/binary.go` around lines 94 - 111, The deadline check in
RetryConfig.checkDeadline currently treats now==deadline as allowed because it
uses !time.Now().After(cfg.Deadline); change the logic so equality is considered
expired by returning nil only when Deadline is zero or now is strictly before
the deadline (use time.Now().Before(cfg.Deadline) instead of
!time.Now().After(...)); update RetryConfig.checkDeadline to enforce expiration
on equality and keep the returned error using ErrInjectTimeout and the
attemptsCompleted value unchanged.
Comment on lines
+142
to
+176
| type BinarySource interface { | ||
| GetBinary(ctx context.Context, arch string) (io.ReadCloser, error) | ||
| SourceName() string | ||
| } | ||
|
|
||
| type BinaryManager struct { | ||
| sources []BinarySource | ||
| logger log.Logger | ||
| } | ||
|
|
||
| func NewBinaryManager(logger log.Logger, downloadURL string) *BinaryManager { | ||
| cachePath := filepath.Join(os.TempDir(), "devpod-cache") | ||
| cache := &BinaryCache{BaseDir: cachePath} | ||
|
|
||
| return &BinaryManager{ | ||
| sources: []BinarySource{ | ||
| &InjectSource{}, | ||
| &FileCacheSource{Cache: cache}, | ||
| &HTTPDownloadSource{BaseURL: downloadURL, Cache: cache}, | ||
| }, | ||
| logger: logger, | ||
| } | ||
| } | ||
|
|
||
| func (m *BinaryManager) AcquireBinary(ctx context.Context, arch string) (io.ReadCloser, string, error) { | ||
| for _, source := range m.sources { | ||
| binary, err := source.GetBinary(ctx, arch) | ||
| if err == nil { | ||
| m.logger.Debugf("acquired binary from %s", source.SourceName()) | ||
| return binary, source.SourceName(), nil | ||
| } | ||
| m.logger.Debugf("source %s failed: %v", source.SourceName(), err) | ||
| } | ||
| return nil, "", ErrBinaryNotFound | ||
| } |
There was a problem hiding this comment.
Stop acquisition when context is canceled.
Line 167 iterates sources without checking ctx; FileCacheSource ignores ctx, so cancellation can be bypassed and a binary returned after cancel/timeout.
🛠️ Suggested fix
func (m *BinaryManager) AcquireBinary(ctx context.Context, arch string) (io.ReadCloser, string, error) {
for _, source := range m.sources {
+ if err := checkContextCancelled(ctx); err != nil {
+ return nil, "", err
+ }
binary, err := source.GetBinary(ctx, arch)
if err == nil {
m.logger.Debugf("acquired binary from %s", source.SourceName())
return binary, source.SourceName(), nil
}
+ if err := checkContextCancelled(ctx); err != nil {
+ return nil, "", err
+ }
m.logger.Debugf("source %s failed: %v", source.SourceName(), err)
}
return nil, "", ErrBinaryNotFound
}🤖 Prompt for AI Agents
In `@pkg/agent/binary.go` around lines 142 - 176, AcquireBinary currently loops
all sources and calls GetBinary without checking ctx, so a canceled/expired
context can be bypassed (notably FileCacheSource ignores ctx) and return a
binary after cancellation; update BinaryManager.AcquireBinary to check ctx
before each iteration (select on ctx.Done()) and immediately return nil, "",
ctx.Err() when canceled, and ensure GetBinary implementations (e.g.,
FileCacheSource.GetBinary) observe the passed ctx where blocking I/O or sleeps
occur so they can return early on ctx cancellation.
This was referenced Feb 5, 2026
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
1 participant
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.
Summary by CodeRabbit