fix: use manual retry loop in bash download() with user feedback#2472
Merged
Conversation
Replace curl --retry / wget --tries with a manual retry loop that provides clear feedback on each failed attempt. This matches the PowerShell Invoke-Download behavior already on main: - Shows 'Download attempt N/M failed, retrying in Xs...' on stderr - Uses exponential backoff (1, 2, 4, 8s) by default - Respects JBANG_DOWNLOAD_RETRY_DELAY for fixed delay override The built-in curl/wget retry flags were problematic: - curl --retry-delay 0 means exponential backoff, but wget --waitretry=0 means no delay at all - Neither tool provides user-visible progress on retries
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe bootstrap scripts replace curl/wget built-in retries with a script-controlled attempt loop (fixed or exponential backoff) and add a configurable JBang releases base URL; they also update the “Downloading JBang …” status to show the resolved download version (defaults to latest) and the resolved download URL. ChangesBootstrap download updates
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/scripts/jbang (1)
12-14:⚠️ Potential issue | 🟠 MajorAdd
JBANG_DOWNLOAD_RETRY/JBANG_DOWNLOAD_RETRY_DELAYdefaults tosrc/main/scripts/jbang.cmd
src/main/scripts/jbang.cmddoes not define default values forJBANG_DOWNLOAD_RETRYorJBANG_DOWNLOAD_RETRY_DELAY, violating the guideline that newJBANG_*env vars must have defaults in all three startup scripts. Add defaults (e.g.,5and0) when unset, matchingsrc/main/scripts/jbang/PowerShell.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/scripts/jbang` around lines 12 - 14, Add default fallbacks for the JBANG_DOWNLOAD_RETRY and JBANG_DOWNLOAD_RETRY_DELAY environment variables in the jbang.cmd startup script: detect if %JBANG_DOWNLOAD_RETRY% and %JBANG_DOWNLOAD_RETRY_DELAY% are empty/unset and set them to 5 and 0 respectively; mirror the defaults used in src/main/scripts/jbang (variables downloadRetry and downloadRetryDelay) so all three startup scripts provide consistent JBANG_* defaults.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/main/scripts/jbang`:
- Around line 12-14: Add default fallbacks for the JBANG_DOWNLOAD_RETRY and
JBANG_DOWNLOAD_RETRY_DELAY environment variables in the jbang.cmd startup
script: detect if %JBANG_DOWNLOAD_RETRY% and %JBANG_DOWNLOAD_RETRY_DELAY% are
empty/unset and set them to 5 and 0 respectively; mirror the defaults used in
src/main/scripts/jbang (variables downloadRetry and downloadRetryDelay) so all
three startup scripts provide consistent JBANG_* defaults.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 52034760-023f-4c37-8b0c-72afb52df455
📒 Files selected for processing (1)
src/main/scripts/jbang
Show the resolved URL in the download message so users can see exactly where JBang is being downloaded from. Helps with debugging mirror/proxy issues and custom JBANG_DOWNLOAD_URL overrides. Before: Downloading JBang 0.138.0... After: Downloading JBang 0.138.0 from https://github.com/.../jbang.tar...
maxandersen
force-pushed
the
retry-download-feedback
branch
5 times, most recently
from
d01c485 to
fa4b769
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/main/scripts/jbang (1)
56-59: ⚡ Quick winConsolidate redundant retry messages on first failure.
On the first download failure, both the hint message (line 57) and the attempt/max message (line 59) are printed, which clutters stderr output. Consider consolidating them into a single message or making them mutually exclusive.
♻️ Proposed consolidation
Option 1: Conditional detailed message on first failure only
if [ $attempt -eq 1 ]; then - echo "Download failed. Retrying... (JBANG_DOWNLOAD_RETRY=0 to disable)" 1>&2 + echo "Download $attempt/$maxAttempts failed. Retry in $sleepSeconds second(s)... (Set JBANG_DOWNLOAD_RETRY=0 to disable)" 1>&2 +else + echo "Download $attempt/$maxAttempts failed. Retry in $sleepSeconds second(s)..." 1>&2 fi -echo "Download $attempt/$maxAttempts failed. Retry in $sleepSeconds second(s)..." 1>&2Option 2: Always show attempt/max, add hint below on first failure
+echo "Download $attempt/$maxAttempts failed. Retry in $sleepSeconds second(s)..." 1>&2 if [ $attempt -eq 1 ]; then - echo "Download failed. Retrying... (JBANG_DOWNLOAD_RETRY=0 to disable)" 1>&2 + echo "(Set JBANG_DOWNLOAD_RETRY=0 to disable retries)" 1>&2 fi -echo "Download $attempt/$maxAttempts failed. Retry in $sleepSeconds second(s)..." 1>&2🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/scripts/jbang` around lines 56 - 59, Consolidate the two stderr retry prints so they aren't both emitted on the first failure: modify the retry logic around the variables attempt, maxAttempts, and sleepSeconds to print a single message—on attempt == 1 emit a combined message that includes the retry hint about JBANG_DOWNLOAD_RETRY and the "Download X/Y failed. Retry in Z second(s)..." text; on subsequent attempts emit only the "Download X/Y failed..." line. Ensure the conditional uses attempt to choose between the combined hint+attempt message and the plain attempt/max message.src/main/scripts/jbang.ps1 (1)
90-93: ⚡ Quick winConsolidate redundant retry messages on first failure.
On the first download failure, both the hint message (line 91) and the attempt/max message (line 93) are printed, which clutters stderr output. Consider consolidating them into a single message or making them mutually exclusive, matching the suggestion for the bash script.
♻️ Proposed consolidation
Option 1: Conditional detailed message on first failure only
if ($attempt -eq 1) { - [Console]::Error.WriteLine("Download failed. Retrying... (JBANG_DOWNLOAD_RETRY=0 to disable)") + [Console]::Error.WriteLine("Download $attempt/$($downloadRetry + 1) failed. Retry in $sleepSeconds second(s)... (Set JBANG_DOWNLOAD_RETRY=0 to disable)") +} else { + [Console]::Error.WriteLine("Download $attempt/$($downloadRetry + 1) failed. Retry in $sleepSeconds second(s)...") } -[Console]::Error.WriteLine("Download $attempt/$($downloadRetry + 1) failed. Retry in $sleepSeconds second(s)...")Option 2: Always show attempt/max, add hint below on first failure
+[Console]::Error.WriteLine("Download $attempt/$($downloadRetry + 1) failed. Retry in $sleepSeconds second(s)...") if ($attempt -eq 1) { - [Console]::Error.WriteLine("Download failed. Retrying... (JBANG_DOWNLOAD_RETRY=0 to disable)") + [Console]::Error.WriteLine("(Set JBANG_DOWNLOAD_RETRY=0 to disable retries)") } -[Console]::Error.WriteLine("Download $attempt/$($downloadRetry + 1) failed. Retry in $sleepSeconds second(s)...")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/scripts/jbang.ps1` around lines 90 - 93, The two stderr writes on first failure are redundant: when $attempt -eq 1 the block prints both the hint and the attempt/max line; update the retry messaging so they are mutually exclusive or consolidated — e.g., inside the $attempt conditional only call [Console]::Error.WriteLine once (combining the hint about JBANG_DOWNLOAD_RETRY with the attempt/$($downloadRetry + 1) and $sleepSeconds info) or skip printing the second attempt/max line when $attempt -eq 1; adjust the code around the $attempt check that calls [Console]::Error.WriteLine and reference the variables $attempt, $downloadRetry, $sleepSeconds and the JBANG_DOWNLOAD_RETRY hint when composing the single message.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/main/scripts/jbang`:
- Around line 56-59: Consolidate the two stderr retry prints so they aren't both
emitted on the first failure: modify the retry logic around the variables
attempt, maxAttempts, and sleepSeconds to print a single message—on attempt == 1
emit a combined message that includes the retry hint about JBANG_DOWNLOAD_RETRY
and the "Download X/Y failed. Retry in Z second(s)..." text; on subsequent
attempts emit only the "Download X/Y failed..." line. Ensure the conditional
uses attempt to choose between the combined hint+attempt message and the plain
attempt/max message.
In `@src/main/scripts/jbang.ps1`:
- Around line 90-93: The two stderr writes on first failure are redundant: when
$attempt -eq 1 the block prints both the hint and the attempt/max line; update
the retry messaging so they are mutually exclusive or consolidated — e.g.,
inside the $attempt conditional only call [Console]::Error.WriteLine once
(combining the hint about JBANG_DOWNLOAD_RETRY with the attempt/$($downloadRetry
+ 1) and $sleepSeconds info) or skip printing the second attempt/max line when
$attempt -eq 1; adjust the code around the $attempt check that calls
[Console]::Error.WriteLine and reference the variables $attempt, $downloadRetry,
$sleepSeconds and the JBANG_DOWNLOAD_RETRY hint when composing the single
message.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f7b40b7c-1cef-4a84-99b7-1e5c43b5c405
📒 Files selected for processing (2)
src/main/scripts/jbangsrc/main/scripts/jbang.ps1
maxandersen
force-pushed
the
retry-download-feedback
branch
2 times, most recently
from
2e88486 to
f03221f
Compare
Add JBANG_DOWNLOAD_BASEURL env var to bash and PowerShell scripts, allowing the download base URL to be overridden. This enables: - Pointing scripts at WireMock/local HTTP servers for integration tests - Corporate mirror support - file:// URL testing Defaults to https://github.com/jbangdev/jbang/releases when unset. JBANG_DOWNLOAD_URL (full URL override) still takes precedence.
maxandersen
force-pushed
the
retry-download-feedback
branch
from
f03221f to
7defd27
Compare
Only require defaults in scripts that actually use the variable, not blindly in all three. jbang.cmd delegates downloads to jbang.ps1, so download-related env vars don't need defaults there.
maxandersen
force-pushed
the
retry-download-feedback
branch
from
27c05b8 to
c2ca48a
Compare
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
2 participants
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.
Improves download feedback and testability in the startup scripts.
Manual retry loop with feedback (bash)
Replace curl
--retry/ wget--trieswith a manual retry loop that provides clear feedback on each failed attempt. This matches the PowerShellInvoke-Downloadbehavior already on main.Download attempt N/M failed, retrying in Xs...on stderrJBANG_DOWNLOAD_RETRY_DELAYfor fixed delay overrideWhy: The built-in curl/wget retry flags had inconsistent behavior (
curl --retry-delay 0= exponential backoff,wget --waitretry=0= no delay). Neither provided user-visible feedback.Download URL in feedback (bash + ps1)
Print the resolved URL in the download message:
Downloading JBang 0.138.0...Downloading JBang 0.138.0 from https://github.com/.../jbang.tar...JBANG_DOWNLOAD_BASEURLsupport (bash + ps1)Add
JBANG_DOWNLOAD_BASEURLenv var to override the download base URL. Enables:file://URL testingDefaults to
https://github.com/jbangdev/jbang/releaseswhen unset.JBANG_DOWNLOAD_URL(full URL override) still takes precedence.Testability env vars after this PR
JBANG_DOWNLOAD_BASEURLJBANG_DOWNLOAD_URLJBANG_DOWNLOAD_VERSIONJBANG_DOWNLOAD_RETRYJBANG_DOWNLOAD_RETRY_DELAYSplit from #2467.
Summary by CodeRabbit
New Features
Bug Fixes