Add retry with backoff for component git fetch by claude[bot] · Pull Request #208 · theonestack/cfhighlander

claude · 2026-09-07T13:40:40Z

What

TemplateFinder#findTemplateGit (lib/cfhighlander.factory.templatefinder.rb) resolves hl-component-* dependencies with a single shallow Git.clone call. Any transient failure from that call (network blip, flaky remote, etc.) is caught by the method's outer rescue Exception, logged, and turned into a nil return — which the caller (ComponentFactory#loadComponentFromTemplate) turns into an unrescued StandardError, crashing the whole CLI run for what is often a one-off, retryable failure.

This adds a small retry-with-exponential-backoff loop around just the Git.clone call:

Configurable via two new env vars, following the existing CFHIGHLANDER_* convention (see CFHIGHLANDER_WORKDIR, CFHIGHLANDER_AWS_RETRY_LIMIT, CFHIGHLANDER_DEFAULT_COMPONENT_GIT_LOOKUP in the same file/README):
- CFHIGHLANDER_COMPONENT_FETCH_RETRIES — number of retries (default 3)
- CFHIGHLANDER_COMPONENT_FETCH_RETRY_DELAY — base delay in seconds for exponential backoff (default 1, so delays are 1s, 2s, 4s, ...)
Each retry attempt is logged to STDERR.
Once retries are exhausted, the original exception is re-raised into the method's existing outer rescue, so behavior after giving up is unchanged: log Failed to resolve component ... and return nil. Callers see exactly the same failure mode as before, just after more attempts.
No new dependency — plain sleep.
The S3 fetch path (findTemplateS3) and the default lookup URL resolution are untouched.

Documented the two new env vars in the README's existing "Environment variables" section, next to CFHIGHLANDER_AWS_RETRY_LIMIT.

How

Added spec/test_templatefinder_spec.rb with two cases against findTemplateGit, stubbing Git.clone and sleep (no real network calls or real waiting):

clone fails a couple of times then succeeds → the component still resolves correctly, and sleep was called with the expected backoff delays (1, then 2).
clone always fails → after exhausting retries the method still returns nil and still logs the original Failed to resolve component ... message, matching current behavior.

Ran the full non-network spec suite locally (bundle exec rspec spec/test_cloudformation_util_spec.rb spec/test_templatefinder_spec.rb) — 3 examples, 0 failures. spec/test_onestackorg_spec.rb was left out of the local run since it needs live GitHub API access and clones several real theonestack/hl-component-* repos; it isn't touched by this change.

Also ran ruby -c on the changed file (syntax OK).

Component fetch shells out to a single Git.clone call with no retry logic, so any transient git/network failure while cloning an hl-component-* repo bubbles up as an unrescued exception in the caller and crashes the whole cfhighlander run. Wrap the clone call with a small retry loop using exponential backoff, configurable via CFHIGHLANDER_COMPONENT_FETCH_RETRIES (default 3) and CFHIGHLANDER_COMPONENT_FETCH_RETRY_DELAY (default 1 second), following the existing CFHIGHLANDER_* env var convention. Each retry is logged to STDERR. Once retries are exhausted the existing failure handling is unchanged: log and return nil. Add a focused spec covering both the retry-then-succeed path and the give-up-after-exhausting-retries path, stubbing sleep so the spec runs instantly.

Copilot

🟡 Changes recommended

Exception handling, configuration validation, and test environment isolation need correction.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Adds configurable retry with exponential backoff for failed component Git clones.

Changes:

Retries Git.clone with configurable limits and delays.
Documents the new environment variables.
Adds success and exhaustion tests.

File summaries

File	Description
`lib/cfhighlander.factory.templatefinder.rb`	Implements clone retries and backoff.
`README.md`	Documents retry configuration.
`spec/test_templatefinder_spec.rb`	Tests successful retries and exhaustion.

Review details

Files reviewed: 3/3 changed files
Comments generated: 3
Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

+          max_retries = (ENV['CFHIGHLANDER_COMPONENT_FETCH_RETRIES'] || 3).to_i
+          retry_delay = (ENV['CFHIGHLANDER_COMPONENT_FETCH_RETRY_DELAY'] || 1).to_i


+          attempt = 0
+          begin
+            Git.clone git_url, cache_path, clone_opts
+          rescue Exception => clone_error


+  around(:each) do |example|
+    Dir.mktmpdir do |dir|
+      @cache_path = File.join(dir, 'component')
+      example.run
+    end
+  end
+
+  before(:each) do
+    # keep the spec instant regardless of retry/delay env overrides
+    allow(finder).to receive(:sleep)
+    ENV.delete('CFHIGHLANDER_COMPONENT_FETCH_RETRIES')
+    ENV.delete('CFHIGHLANDER_COMPONENT_FETCH_RETRY_DELAY')
+  end


- Narrow the retry loop's rescue clause to StandardError instead of Exception, so process-control signals like Interrupt/SystemExit during a clone are no longer swallowed and retried. - Validate CFHIGHLANDER_COMPONENT_FETCH_RETRIES and CFHIGHLANDER_COMPONENT_FETCH_RETRY_DELAY as non-negative integers, falling back to their documented defaults (3 and 1) on invalid, negative or non-numeric input instead of letting a bad value reach sleep and raise ArgumentError. - Restore the original env var values after each spec example via an around hook, and add coverage for the new fallback behavior.

Copilot

🟢 Approval recommended

The implementation is sound, with only a minor gap covering valid custom delay values.

Review details

Files reviewed: 3/3 changed files
Comments generated: 1
Review effort level: Balanced

+  it "retries a failing clone and succeeds once it stops failing" do
+    ENV['CFHIGHLANDER_COMPONENT_FETCH_RETRIES'] = '3'
+
+    attempts = 0
+    allow(Git).to receive(:clone) do |_url, path, _opts|
+      attempts += 1
+      raise Git::GitExecuteError, "simulated github flakiness" if attempts < 3
+
+      FileUtils.mkdir_p path
+      File.write(File.join(path, 'mycomponent.cfhighlander.rb'), '')
+    end
+
+    component_name, location = finder.findTemplateGit(
+      @cache_path, 'mycomponent', 'latest', 'https://github.com/theonestack/hl-component-mycomponent', 'master'
+    )
+
+    expect(attempts).to eq(3)
+    expect(component_name).to eq('mycomponent')
+    expect(location).to eq("#{@cache_path}/")
+    expect(finder).to have_received(:sleep).twice
+    expect(finder).to have_received(:sleep).with(1)
+    expect(finder).to have_received(:sleep).with(2)
+  end


The existing tests only covered the default retry delay and the invalid-value fallback, so a custom CFHIGHLANDER_COMPONENT_FETCH_RETRY_DELAY override was never actually exercised. Add a test that sets the delay to a non-default value and asserts the exponential backoff sequence (delay, delay*2) it produces.

Copilot

🟢 Approval recommended

The implementation is focused, preserves existing failure behavior, and has adequate automated coverage.

Review details

Files reviewed: 3/3 changed files
Comments generated: 0 new
Review effort level: Balanced

claude Bot requested a review from jaredbrook September 7, 2026 13:40

jaredbrook requested a balanced review from Copilot September 7, 2026 21:45

Copilot started reviewing on behalf of jaredbrook September 7, 2026 21:46

View session

Copilot AI reviewed Sep 7, 2026

View reviewed changes

jaredbrook requested a balanced review from Copilot September 7, 2026 22:05

Copilot started reviewing on behalf of jaredbrook September 7, 2026 22:05

View session

Copilot AI reviewed Sep 7, 2026

View reviewed changes

jaredbrook requested a balanced review from Copilot September 7, 2026 22:25

Copilot started reviewing on behalf of jaredbrook September 7, 2026 22:25

View session

Copilot AI reviewed Sep 7, 2026

View reviewed changes

jaredbrook approved these changes Sep 7, 2026

View reviewed changes

jaredbrook merged commit 518277c into master Sep 7, 2026
4 checks passed

claude Bot mentioned this pull request Sep 7, 2026

Bump version to 0.13.8 #209

Merged

Navigation Menu

Repository navigation

Add retry with backoff for component git fetch - #208

Add retry with backoff for component git fetch#208
jaredbrook merged 3 commits into
masterfrom
feature/retry-github-component-fetch

claude Bot commented Sep 7, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		max_retries = (ENV['CFHIGHLANDER_COMPONENT_FETCH_RETRIES'] \|\| 3).to_i
		retry_delay = (ENV['CFHIGHLANDER_COMPONENT_FETCH_RETRY_DELAY'] \|\| 1).to_i

Conversation

claude Bot commented Sep 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What

How

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟢 Approval recommended

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟢 Approval recommended

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

claude Bot commented Sep 7, 2026 •

edited

Loading