fix: more resilient apt commands #51
Conversation
|
I just wanted to +1 this PR — we’re encountering the same intermittent issue on our self-hosted runners. |
|
This pull request is stale because it has been open 5 days with no activity. Remove stale label or comment or this will be closed in 5 days. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for i in $(seq 1 $max_retries); do | ||
| if sudo apt-get update; then | ||
| echo "apt-get update succeeded" | ||
| break | ||
| else | ||
| echo "apt-get update failed, retrying ($i/$max_retries) in $delay seconds..." | ||
| sleep $delay | ||
| delay=$((delay * 2)) # exponential backoff | ||
| fi | ||
| done |
There was a problem hiding this comment.
The retry loop doesn't check if apt-get update ultimately succeeded after all retries are exhausted. If all 5 attempts fail, the script continues to the install step which may fail or produce unexpected results. Consider adding a check after the loop:
if [ $i -eq $max_retries ] && ! sudo apt-get update; then
echo "apt-get update failed after $max_retries attempts"
exit 1
fiAlternatively, track success in a variable inside the loop and check it afterward.
| max_retries=5 | ||
| delay=5 | ||
| for i in $(seq 1 $max_retries); do | ||
| if sudo apt-get update; then |
There was a problem hiding this comment.
The retry logic updates all apt repositories, but the original code specifically updated only the Twingate repository using -o Dir::Etc::sourcelist="sources.list.d/twingate.list" -o Dir::Etc::sourceparts="-" -o APT::Get::List-Cleanup="0". This change could unnecessarily update all system repositories on each retry, potentially increasing execution time and exposing the action to unrelated apt repository issues.
Consider restoring the targeted update to maintain the original focused approach:
if sudo apt-get update -o Dir::Etc::sourcelist="sources.list.d/twingate.list" -o Dir::Etc::sourceparts="-" -o APT::Get::List-Cleanup="0"; then| if sudo apt-get update; then | |
| if sudo apt-get update -o Dir::Etc::sourcelist="sources.list.d/twingate.list" -o Dir::Etc::sourceparts="-" -o APT::Get::List-Cleanup="0"; then |
| else | ||
| echo "apt-get update failed, retrying ($i/$max_retries) in $delay seconds..." | ||
| sleep $delay | ||
| delay=$((delay * 2)) # exponential backoff |
There was a problem hiding this comment.
The delay is doubled on each retry but only after sleeping, which means on the final (5th) retry attempt, the delay is calculated to 80 seconds but never used. Consider moving the delay calculation before the sleep, or better yet, only increment it after the sleep when there will be a next iteration:
if [ $i -lt $max_retries ]; then
sleep $delay
delay=$((delay * 2))
fiThis makes the logic clearer and avoids unnecessary computation on the final iteration.
| delay=$((delay * 2)) # exponential backoff | |
| if [ $i -lt $max_retries ]; then | |
| delay=$((delay * 2)) # exponential backoff | |
| fi |
|
This pull request is stale because it has been open 5 days with no activity. Remove stale label or comment or this will be closed in 5 days. |
2 participants
We use self-hosted runners and have found that sometimes another process (probably apt-daily) has a lock and causes the
apt updatein this action to fail. Adding an exponential backoff provides more resilience for the action. Adding the-o DPkg::Lock::Timeout=60provides additional resilience so it won't fail immediately if another dpkg process is running.