Conversation
no ref The yarn registry has been flaky. This adds a little retrying in case the registry is down.
WalkthroughThe 🚥 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: 1
🧹 Nitpick comments (1)
.github/scripts/install-deps.sh (1)
25-27: Add jitter to the backoff.The fixed
15/30/45cadence can cause multiple CI jobs to hit the registry again at the same time after a shared outage. A small random offset will spread retries out and improve the odds of recovery.💡 Possible tweak
- sleep_seconds=$((attempt * 15)) + jitter_seconds=$((RANDOM % 10)) + sleep_seconds=$((attempt * 15 + jitter_seconds)) echo "::warning::Dependency installation failed, retrying in ${sleep_seconds} seconds..." sleep "$sleep_seconds"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/scripts/install-deps.sh around lines 25 - 27, The backoff uses a fixed cadence via sleep_seconds=$((attempt * 15)) which can synchronize retries; add a small random jitter when computing sleep_seconds (e.g., compute a jitter value using RANDOM or $RANDOM modulo a small max and add/subtract it) and then use that jittered sleep_seconds in the existing echo and sleep calls so retries are spread out; update the variables sleep_seconds and the sleep invocation (and keep attempt unchanged) to include the jitter.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/scripts/install-deps.sh:
- Around line 16-23: The script currently retries every non-zero exit from
install_dependencies; change install_dependencies to detect transient
registry/network errors (e.g., match yarn/npm stderr for patterns like
ENOTFOUND, EAI_AGAIN, ETIMEDOUT, ECONNREFUSED, 502/503/504/408, "socket hang
up", "ETIMEDOUT", "Request failed" or similar) and return a special retryable
status only for those cases, while returning a non-retryable failure immediately
for deterministic errors (lockfile drift, authentication, package metadata
errors); update the loop that checks install_dependencies (the code using
attempt and max_attempts) to only retry when install_dependencies indicates a
transient/network error and fail fast on non-retryable failures.
---
Nitpick comments:
In @.github/scripts/install-deps.sh:
- Around line 25-27: The backoff uses a fixed cadence via
sleep_seconds=$((attempt * 15)) which can synchronize retries; add a small
random jitter when computing sleep_seconds (e.g., compute a jitter value using
RANDOM or $RANDOM modulo a small max and add/subtract it) and then use that
jittered sleep_seconds in the existing echo and sleep calls so retries are
spread out; update the variables sleep_seconds and the sleep invocation (and
keep attempt unchanged) to include the jitter.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7948207d-c048-46b1-bfba-7ad2502c27d6
📒 Files selected for processing (1)
.github/scripts/install-deps.sh
| if install_dependencies "$@"; then | ||
| break | ||
| fi | ||
|
|
||
| if [ "$attempt" -eq "$max_attempts" ]; then | ||
| echo "Dependency installation failed after ${max_attempts} attempts" | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
Retry only transient install failures.
Any non-zero yarn install exit now gets retried, including deterministic failures like lockfile drift, bad package metadata, or auth/config issues. That will slow down broken PRs and bury the real failure behind three extra attempts. Please gate retries on transient registry/network errors and fail fast otherwise.
💡 Possible fix
for attempt in $(seq 1 "$max_attempts"); do
echo "Installing dependencies with --ignore-scripts... (attempt ${attempt}/${max_attempts})"
- if install_dependencies "$@"; then
+ log_file="$(mktemp)"
+ if install_dependencies "$@" 2>&1 | tee "$log_file"; then
+ rm -f "$log_file"
break
fi
+
+ if ! grep -Eqi 'EAI_AGAIN|ECONNRESET|ETIMEDOUT|ESOCKETTIMEDOUT|502 Bad Gateway|503 Service Unavailable' "$log_file"; then
+ rm -f "$log_file"
+ echo "Dependency installation failed with a non-transient error; not retrying"
+ exit 1
+ fi
+ rm -f "$log_file"
if [ "$attempt" -eq "$max_attempts" ]; then
echo "Dependency installation failed after ${max_attempts} attempts"
exit 1
fi📝 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.
| if install_dependencies "$@"; then | |
| break | |
| fi | |
| if [ "$attempt" -eq "$max_attempts" ]; then | |
| echo "Dependency installation failed after ${max_attempts} attempts" | |
| exit 1 | |
| fi | |
| log_file="$(mktemp)" | |
| if install_dependencies "$@" 2>&1 | tee "$log_file"; then | |
| rm -f "$log_file" | |
| break | |
| fi | |
| if ! grep -Eqi 'EAI_AGAIN|ECONNRESET|ETIMEDOUT|ESOCKETTIMEDOUT|502 Bad Gateway|503 Service Unavailable' "$log_file"; then | |
| rm -f "$log_file" | |
| echo "Dependency installation failed with a non-transient error; not retrying" | |
| exit 1 | |
| fi | |
| rm -f "$log_file" | |
| if [ "$attempt" -eq "$max_attempts" ]; then | |
| echo "Dependency installation failed after ${max_attempts} attempts" | |
| exit 1 | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/scripts/install-deps.sh around lines 16 - 23, The script currently
retries every non-zero exit from install_dependencies; change
install_dependencies to detect transient registry/network errors (e.g., match
yarn/npm stderr for patterns like ENOTFOUND, EAI_AGAIN, ETIMEDOUT, ECONNREFUSED,
502/503/504/408, "socket hang up", "ETIMEDOUT", "Request failed" or similar) and
return a special retryable status only for those cases, while returning a
non-retryable failure immediately for deterministic errors (lockfile drift,
authentication, package metadata errors); update the loop that checks
install_dependencies (the code using attempt and max_attempts) to only retry
when install_dependencies indicates a transient/network error and fail fast on
non-retryable failures.
There was a problem hiding this comment.
Deciding not to address this.
There was a problem hiding this comment.
@EvanHahn Understood, no worries! Happy to defer to your judgment on this one.
🐰 hops away quietly
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #26781 +/- ##
==========================================
- Coverage 73.20% 73.20% -0.01%
==========================================
Files 1534 1534
Lines 120966 121034 +68
Branches 14630 14635 +5
==========================================
+ Hits 88559 88597 +38
- Misses 31393 31423 +30
Partials 1014 1014
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This comment was marked as outdated.
This comment was marked as outdated.
3 participants
no ref
The yarn registry has been flaky. This adds a little retrying in case the registry is down.