Skip to content

Commit 8cb8268

Browse files
jh-lee-cryptolabjh-lee-cryptolab
authored
Merge pull request #165 from jh-lee-cryptolab/fix/bootstrap-retry-504
fix(bootstrap): retry transient network failures during install
2 parents c1ae2cf + edcdb54 commit 8cb8268

5 files changed

Lines changed: 141 additions & 29 deletions

File tree

‎bin/rune‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -86,9 +86,11 @@ if [ -z "$RUNE_VERSION" ]; then
8686
token="${GITHUB_TOKEN:-${GH_TOKEN:-}}"
8787
if [ -n "$token" ]; then
8888
body="$(curl --fail --silent --show-error --location --connect-timeout 10 --max-time 20 \
89+
--retry 3 --retry-delay 2 \
8990
--header "Authorization: Bearer $token" "$api" || true)"
9091
else
91-
body="$(curl --fail --silent --show-error --location --connect-timeout 10 --max-time 20 "$api" || true)"
92+
body="$(curl --fail --silent --show-error --location --connect-timeout 10 --max-time 20 \
93+
--retry 3 --retry-delay 2 "$api" || true)"
9294
fi
9395

9496
RUNE_VERSION="$(printf '%s' "$body" \
@@ -126,8 +128,10 @@ mkdir -p "$(dirname "$TARGET")"
126128
TMP="$(mktemp "$(dirname "$TARGET")/.rune-bootstrap-XXXXXX")"
127129
SUMS="$(mktemp -t rune-bootstrap-sums-XXXXXX)"
128130

129-
curl --fail --silent --show-error --location --connect-timeout 10 --max-time 120 "$RELEASE_BASE/$ASSET" -o "$TMP"
130-
curl --fail --silent --show-error --location --connect-timeout 10 --max-time 30 "$RELEASE_BASE/checksums.txt" -o "$SUMS"
131+
# --retry rides out transient GitHub CDN failures (504, timeouts) instead
132+
# of aborting the whole bootstrap on the first blip.
133+
curl --fail --silent --show-error --location --connect-timeout 10 --max-time 120 --retry 3 --retry-delay 2 "$RELEASE_BASE/$ASSET" -o "$TMP"
134+
curl --fail --silent --show-error --location --connect-timeout 10 --max-time 30 --retry 3 --retry-delay 2 "$RELEASE_BASE/checksums.txt" -o "$SUMS"
131135

132136
EXPECTED="$(grep " $ASSET\$" "$SUMS" | cut -d' ' -f1)"
133137
if [ -z "$EXPECTED" ]; then

‎internal/bootstrap/download.go‎

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,53 @@ func newDownloadTransport() *http.Transport {
3030

3131
type ProgressFunc func(downloaded, total int64)
3232

33+
// maxDownloadAttempts caps retries for a single network fetch (manifest
34+
// or artifact). A transient blip — most notably a GitHub CDN 504 during
35+
// install — recovers within a couple of attempts; beyond that the
36+
// failure is most likely persistent (wrong SHA, missing asset) where
37+
// further retries only add latency.
38+
const maxDownloadAttempts = 3
39+
40+
// downloadRetryBackoff is the initial wait between attempts; it is
41+
// multiplied by retryBackoffMultiplier each retry so a transient blip
42+
// recovers quickly while a server-side cold-start has time to warm up.
43+
// var so tests can compress the real waits.
44+
var downloadRetryBackoff = 2 * time.Second
45+
46+
const retryBackoffMultiplier = 3
47+
48+
// withRetry runs fn up to maxDownloadAttempts times with bounded
49+
// exponential backoff so a transient network failure (e.g. a GitHub CDN
50+
// 504) surfaces as a retry rather than a hard install failure. ctx
51+
// cancellation aborts immediately without burning the remaining
52+
// attempts. logf may be nil.
53+
func withRetry(ctx context.Context, logf func(string, ...any), label string, fn func() error) error {
54+
var lastErr error
55+
backoff := downloadRetryBackoff
56+
for attempt := 1; attempt <= maxDownloadAttempts; attempt++ {
57+
if attempt > 1 {
58+
if logf != nil {
59+
logf("retrying %s (attempt %d/%d) after %s: %v", label, attempt, maxDownloadAttempts, backoff, lastErr)
60+
}
61+
select {
62+
case <-time.After(backoff):
63+
case <-ctx.Done():
64+
return fmt.Errorf("%s: retry aborted: %w", label, ctx.Err())
65+
}
66+
backoff *= retryBackoffMultiplier
67+
}
68+
if err := fn(); err != nil {
69+
if ctx.Err() != nil {
70+
return err
71+
}
72+
lastErr = err
73+
continue
74+
}
75+
return nil
76+
}
77+
return fmt.Errorf("%s: failed after %d attempts: %w", label, maxDownloadAttempts, lastErr)
78+
}
79+
3380
func DownloadAndVerify(ctx context.Context, spec ArtifactSpec, destPath string, progress ProgressFunc) error {
3481
if spec.URL == "" || spec.SHA256 == "" {
3582
return errors.New("download: spec missing url or sha256")

‎internal/bootstrap/install.go‎

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -76,7 +76,7 @@ func Install(ctx context.Context, opts InstallOptions) (*Result, error) {
7676

7777
// Fetch manifest
7878
logf("[1/%d] manifest: fetching from %s", total, opts.ManifestURL)
79-
manifest, err := FetchManifest(ctx, opts.ManifestURL)
79+
manifest, err := FetchManifest(ctx, opts.ManifestURL, logf)
8080
if err != nil {
8181
r.Failed[StepManifest] = err.Error()
8282
r.Status = "partial"
@@ -123,7 +123,7 @@ func Install(ctx context.Context, opts InstallOptions) (*Result, error) {
123123
}
124124

125125
logf("[%d/%d] %s (%d bytes): downloading...", stepNum, total, in.step, in.spec.Size)
126-
if err := installArtifact(ctx, paths, in.spec, in.dest, opts.Progress); err != nil {
126+
if err := installArtifact(ctx, paths, in.spec, in.dest, opts.Progress, logf); err != nil {
127127
r.Failed[in.step] = err.Error()
128128
r.Status = "partial"
129129
return r, err
@@ -198,10 +198,12 @@ func onlySkipped(r *Result) bool {
198198
// spec.Extract == "" : raw binaries are downloaded directly to dest
199199
// spec.Extract == "tar.gz": archive is extracted into dest
200200
// Others treat as error
201-
func installArtifact(ctx context.Context, paths *Paths, spec ArtifactSpec, dest string, progress ProgressFunc) error {
201+
func installArtifact(ctx context.Context, paths *Paths, spec ArtifactSpec, dest string, progress ProgressFunc, logf func(string, ...any)) error {
202202
switch spec.Extract {
203203
case "": // raw binaries
204-
if err := DownloadAndVerify(ctx, spec, dest, progress); err != nil {
204+
if err := withRetry(ctx, logf, "download "+filepath.Base(dest), func() error {
205+
return DownloadAndVerify(ctx, spec, dest, progress)
206+
}); err != nil {
205207
return err
206208
}
207209
if err := os.Chmod(dest, binaryMode); err != nil {
@@ -212,7 +214,9 @@ func installArtifact(ctx context.Context, paths *Paths, spec ArtifactSpec, dest
212214
case "tar.gz": // Tarball archive
213215
// Download archive under paths.Cache first
214216
tarPath := filepath.Join(paths.Cache, filepath.Base(dest)+".tar.gz")
215-
if err := DownloadAndVerify(ctx, spec, tarPath, progress); err != nil {
217+
if err := withRetry(ctx, logf, "download "+filepath.Base(dest), func() error {
218+
return DownloadAndVerify(ctx, spec, tarPath, progress)
219+
}); err != nil {
216220
return err
217221
}
218222
defer os.Remove(tarPath)

‎internal/bootstrap/manifest.go‎

Lines changed: 33 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -54,14 +54,45 @@ var (
5454
ErrNoArtifactForPlatform = errors.New("manifest: no artifacts for this platform")
5555
)
5656

57-
func FetchManifest(ctx context.Context, manifestURL string) (*Manifest, error) {
57+
func FetchManifest(ctx context.Context, manifestURL string, logf func(string, ...any)) (*Manifest, error) {
5858
if v := os.Getenv(envManifest); v != "" {
5959
manifestURL = v
6060
}
6161
if manifestURL == "" {
6262
return nil, errors.New("manifest: no URL provided (default missing; set RUNE_MANIFEST?)")
6363
}
6464

65+
// Only the network fetch is retried; a transient GitHub CDN 504 should
66+
// not abort install, but a parse/version error below is deterministic
67+
// and retrying it would just waste the backoff budget.
68+
var body []byte
69+
if err := withRetry(ctx, logf, "fetch manifest", func() error {
70+
var ferr error
71+
body, ferr = fetchManifestBody(ctx, manifestURL)
72+
return ferr
73+
}); err != nil {
74+
return nil, err
75+
}
76+
77+
var m Manifest
78+
dec := json.NewDecoder(bytes.NewReader(body))
79+
dec.DisallowUnknownFields()
80+
if err := dec.Decode(&m); err != nil {
81+
return nil, fmt.Errorf("manifest: parse JSON: %w", err)
82+
}
83+
if m.Version != ManifestVersion {
84+
return nil, fmt.Errorf("%w: got %d, want %d", ErrUnsupportedManifestVersion, m.Version, ManifestVersion)
85+
}
86+
if len(m.Platforms) == 0 {
87+
return nil, errors.New("manifest: platforms is empty")
88+
}
89+
90+
return &m, nil
91+
}
92+
93+
// fetchManifestBody performs a single manifest GET and returns the raw
94+
// body. It is the retryable unit wrapped by FetchManifest's withRetry.
95+
func fetchManifestBody(ctx context.Context, manifestURL string) ([]byte, error) {
6596
ctx, cancel := context.WithTimeout(ctx, defaultManifestFetchTimeout)
6697
defer cancel()
6798

@@ -88,20 +119,7 @@ func FetchManifest(ctx context.Context, manifestURL string) (*Manifest, error) {
88119
return nil, fmt.Errorf("manifest: body exceeds %d bytes", maxBody)
89120
}
90121

91-
var m Manifest
92-
dec := json.NewDecoder(bytes.NewReader(body))
93-
dec.DisallowUnknownFields()
94-
if err := dec.Decode(&m); err != nil {
95-
return nil, fmt.Errorf("manifest: parse JSON: %w", err)
96-
}
97-
if m.Version != ManifestVersion {
98-
return nil, fmt.Errorf("%w: got %d, want %d", ErrUnsupportedManifestVersion, m.Version, ManifestVersion)
99-
}
100-
if len(m.Platforms) == 0 {
101-
return nil, errors.New("manifest: platforms is empty")
102-
}
103-
104-
return &m, nil
122+
return body, nil
105123
}
106124

107125
func (m *Manifest) ArtifactsForCurrentPlatform() (PlatformArtifacts, error) {

‎internal/bootstrap/manifest_test.go‎

Lines changed: 45 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -8,8 +8,18 @@ import (
88
"net/http/httptest"
99
"strings"
1010
"testing"
11+
"time"
1112
)
1213

14+
// fastRetry compresses the retry backoff so tests that exercise the retry
15+
// path don't sleep for real seconds. It restores the original on cleanup.
16+
func fastRetry(t *testing.T) {
17+
t.Helper()
18+
orig := downloadRetryBackoff
19+
downloadRetryBackoff = time.Millisecond
20+
t.Cleanup(func() { downloadRetryBackoff = orig })
21+
}
22+
1323
func fullManifestJSON(extraFields string) string {
1424
return fmt.Sprintf(`{
1525
"version": 1,
@@ -31,7 +41,7 @@ func TestFetchManifest_HappyPath(t *testing.T) {
3141
}))
3242
defer srv.Close()
3343

34-
m, err := FetchManifest(context.Background(), srv.URL)
44+
m, err := FetchManifest(context.Background(), srv.URL, nil)
3545
if err != nil {
3646
t.Fatalf("FetchManifest: %v", err)
3747
}
@@ -66,7 +76,7 @@ func TestFetchManifest_EnvOverride(t *testing.T) {
6676
defer srv.Close()
6777

6878
t.Setenv(envManifest, srv.URL)
69-
if _, err := FetchManifest(context.Background(), "https://default/manifest.json"); err != nil {
79+
if _, err := FetchManifest(context.Background(), "https://default/manifest.json", nil); err != nil {
7080
t.Fatalf("FetchManifest: %v", err)
7181
}
7282
if !served {
@@ -81,7 +91,7 @@ func TestFetchManifest_RejectsUnsupportedVersion(t *testing.T) {
8191
}))
8292
defer srv.Close()
8393

84-
_, err := FetchManifest(context.Background(), srv.URL)
94+
_, err := FetchManifest(context.Background(), srv.URL, nil)
8595
if !errors.Is(err, ErrUnsupportedManifestVersion) {
8696
t.Fatalf("want ErrUnsupportedManifestVersion, got %v", err)
8797
}
@@ -94,7 +104,7 @@ func TestFetchManifest_RejectsEmptyPlatforms(t *testing.T) {
94104
}))
95105
defer srv.Close()
96106

97-
_, err := FetchManifest(context.Background(), srv.URL)
107+
_, err := FetchManifest(context.Background(), srv.URL, nil)
98108
if err == nil || !strings.Contains(err.Error(), "platforms is empty") {
99109
t.Errorf("want empty-platforms error, got %v", err)
100110
}
@@ -106,24 +116,53 @@ func TestFetchManifest_RejectsUnknownFields(t *testing.T) {
106116
}))
107117
defer srv.Close()
108118

109-
_, err := FetchManifest(context.Background(), srv.URL)
119+
_, err := FetchManifest(context.Background(), srv.URL, nil)
110120
if err == nil || !strings.Contains(err.Error(), "unknown field") {
111121
t.Errorf("want unknown-field rejection, got %v", err)
112122
}
113123
}
114124

115125
func TestFetchManifest_HTTPError(t *testing.T) {
126+
fastRetry(t)
116127
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
117128
http.Error(w, "not found", http.StatusNotFound)
118129
}))
119130
defer srv.Close()
120131

121-
_, err := FetchManifest(context.Background(), srv.URL)
132+
_, err := FetchManifest(context.Background(), srv.URL, nil)
122133
if err == nil || !strings.Contains(err.Error(), "404") {
123134
t.Errorf("want HTTP 404, got %v", err)
124135
}
125136
}
126137

138+
// TestFetchManifest_RetriesTransient verifies a transient 5xx (e.g. a
139+
// GitHub CDN 504) is retried and the fetch ultimately succeeds.
140+
func TestFetchManifest_RetriesTransient(t *testing.T) {
141+
fastRetry(t)
142+
var attempts int
143+
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
144+
attempts++
145+
if attempts < 3 {
146+
http.Error(w, "gateway timeout", http.StatusGatewayTimeout)
147+
return
148+
}
149+
w.Header().Set("Content-Type", "application/json")
150+
_, _ = w.Write([]byte(fullManifestJSON("")))
151+
}))
152+
defer srv.Close()
153+
154+
m, err := FetchManifest(context.Background(), srv.URL, nil)
155+
if err != nil {
156+
t.Fatalf("FetchManifest after transient 504s: %v", err)
157+
}
158+
if m.Version != 1 {
159+
t.Errorf("Version = %d, want 1", m.Version)
160+
}
161+
if attempts != 3 {
162+
t.Errorf("attempts = %d, want 3 (two 504s then success)", attempts)
163+
}
164+
}
165+
127166
func TestArtifactsForCurrentPlatform_NotFound(t *testing.T) {
128167
m := &Manifest{
129168
Version: 1,

0 commit comments

Comments
 (0)