From 23be3f9278ad8de1abfdffe16aa7d747e08d94da Mon Sep 17 00:00:00 2001 From: Samuel K Date: Wed, 22 Jul 2026 23:36:19 -0500 Subject: [PATCH 01/16] fix(vscodeweb): retry VS Code CLI download and report truncation clearly The VS Code CLI was streamed straight from the HTTP response body into the extractor with only a status-code check, so a truncated transfer surfaced as an opaque 'extract VS Code CLI: decompress: ... unexpected EOF' and failed the whole up with no retry. Download to a temp file first and verify the transfer completed (Content-Length when advertised), so an incomplete download is reported as a clear, correctly attributed 'download VS Code CLI: incomplete transfer' error, and an extract failure now genuinely means a corrupt archive. Wrap the download+extract in wait.ExponentialBackoff (matching oci_retry.go in the feature package) so a transient CDN hiccup no longer fails the build. Adds unit tests for success, truncation attribution, error status, and retry recovery. --- pkg/ide/vscodeweb/vscodeweb.go | 90 +++++++++++++++++++--- pkg/ide/vscodeweb/vscodeweb_test.go | 112 ++++++++++++++++++++++++++++ 2 files changed, 191 insertions(+), 11 deletions(-) diff --git a/pkg/ide/vscodeweb/vscodeweb.go b/pkg/ide/vscodeweb/vscodeweb.go index cec708006..a4a56fe42 100644 --- a/pkg/ide/vscodeweb/vscodeweb.go +++ b/pkg/ide/vscodeweb/vscodeweb.go @@ -2,12 +2,14 @@ package vscodeweb import ( "fmt" + "io" "net/http" "os" "os/exec" "path/filepath" "runtime" "strconv" + "time" "github.com/devsy-org/devsy/pkg/command" "github.com/devsy-org/devsy/pkg/config" @@ -18,6 +20,7 @@ import ( "github.com/devsy-org/devsy/pkg/ide/vscode" "github.com/devsy-org/devsy/pkg/log" "github.com/devsy-org/devsy/pkg/util" + "k8s.io/apimachinery/pkg/util/wait" ) // Release URL format for the standalone VS Code CLI. The CLI tarball contains a @@ -126,7 +129,7 @@ func (v *VSCodeWeb) Install() error { if !v.isInstalled(location, releaseURL) { vscode.InstallAPKRequirements() - if err := downloadAndExtract(releaseURL, location); err != nil { + if err := downloadAndExtractWithRetry(releaseURL, location); err != nil { return err } @@ -296,26 +299,91 @@ func (v *VSCodeWeb) installSettings() error { return nil } -// downloadAndExtract fetches the CLI tarball at url and extracts it under -// location. The VS Code CLI tarball holds a single `code` binary at its root, -// so no strip levels are applied. Cleans up a partial extraction on failure so -// retries start fresh. +var vscodeCLIBackoff = wait.Backoff{ + Duration: 1 * time.Second, + Factor: 2.0, + Steps: 3, +} + +// downloadAndExtractWithRetry retries the CLI download and extraction, backing +// off between attempts. Failures here are dominated by transient transfer +// truncation (the CLI is served from a CDN), so a single hiccup should not fail +// the whole up. downloadAndExtract cleans up a partial install on failure, so +// each attempt starts fresh. +func downloadAndExtractWithRetry(url, location string) error { + var lastErr error + err := wait.ExponentialBackoff(vscodeCLIBackoff, func() (bool, error) { + lastErr = downloadAndExtract(url, location) + if lastErr == nil { + return true, nil + } + log.Warnf("VS Code CLI install failed, retrying: %v", lastErr) + return false, nil + }) + if wait.Interrupted(err) { + return lastErr + } + return err +} + +// downloadAndExtract downloads the CLI tarball at url to a temporary file, +// verifies the transfer completed, then extracts it under location. Downloading +// to a file first (rather than streaming the response body into the extractor) +// lets a truncated transfer surface as a clear, retryable error instead of an +// opaque decompression EOF. The VS Code CLI tarball holds a single `code` +// binary at its root, so no strip levels are applied. Cleans up a partial +// extraction on failure so retries start fresh. func downloadAndExtract(url, location string) error { - resp, err := devsyhttp.GetHTTPClient().Get(url) // #nosec G107 -- URL comes from VersionOption. + tmpFile, err := os.CreateTemp("", "vscode-cli-*.tar.gz") if err != nil { + return fmt.Errorf("create temp file for VS Code CLI: %w", err) + } + tmpPath := tmpFile.Name() + defer func() { + _ = tmpFile.Close() + _ = os.Remove(tmpPath) + }() + + if err := downloadToFile(url, tmpFile); err != nil { return err } - defer func() { _ = resp.Body.Close() }() - if resp.StatusCode >= http.StatusBadRequest { - return fmt.Errorf("download VS Code CLI: %s returned %s", url, resp.Status) + if _, err := tmpFile.Seek(0, io.SeekStart); err != nil { + return fmt.Errorf("rewind VS Code CLI download: %w", err) } - if err := extract.Extract(resp.Body, location); err != nil { + if err := extract.Extract(tmpFile, location); err != nil { if rmErr := os.RemoveAll(location); rmErr != nil { log.Warnf("cleanup partial install: path=%s err=%v", location, rmErr) } - return fmt.Errorf("extract VS Code CLI: %w", err) + return fmt.Errorf("extract VS Code CLI (archive may be corrupt): %w", err) + } + return nil +} + +// downloadToFile downloads url into dst, rejecting error statuses and short +// transfers so an incomplete download is reported clearly rather than as a +// later decompression failure. +func downloadToFile(url string, dst io.Writer) error { + resp, err := devsyhttp.GetHTTPClient().Get(url) // #nosec G107 -- URL comes from VersionOption. + if err != nil { + return fmt.Errorf("download VS Code CLI: %w", err) + } + defer func() { _ = resp.Body.Close() }() + + if resp.StatusCode >= http.StatusBadRequest { + return fmt.Errorf("download VS Code CLI: %s returned %s", url, resp.Status) + } + + written, err := io.Copy(dst, resp.Body) + if err != nil { + return fmt.Errorf("download VS Code CLI: %w", err) + } + if resp.ContentLength >= 0 && written != resp.ContentLength { + return fmt.Errorf( + "download VS Code CLI: incomplete transfer, got %d of %d bytes", + written, resp.ContentLength, + ) } return nil } diff --git a/pkg/ide/vscodeweb/vscodeweb_test.go b/pkg/ide/vscodeweb/vscodeweb_test.go index 927e938f7..c26321093 100644 --- a/pkg/ide/vscodeweb/vscodeweb_test.go +++ b/pkg/ide/vscodeweb/vscodeweb_test.go @@ -1,14 +1,126 @@ package vscodeweb import ( + "archive/tar" + "bytes" + "compress/gzip" + "net/http" + "net/http/httptest" "os" + "path/filepath" "runtime" "strings" "testing" + "time" "github.com/devsy-org/devsy/pkg/config" + "k8s.io/apimachinery/pkg/util/wait" ) +// buildCodeTarGz returns a gzipped tar holding a single `code` binary at its +// root, matching the layout of the real VS Code CLI tarball. +func buildCodeTarGz(t *testing.T) []byte { + t.Helper() + var buf bytes.Buffer + gz := gzip.NewWriter(&buf) + tw := tar.NewWriter(gz) + content := []byte("#!/bin/sh\necho code\n") + if err := tw.WriteHeader( + &tar.Header{Name: "code", Mode: 0o755, Size: int64(len(content))}, + ); err != nil { + t.Fatal(err) + } + if _, err := tw.Write(content); err != nil { + t.Fatal(err) + } + if err := tw.Close(); err != nil { + t.Fatal(err) + } + if err := gz.Close(); err != nil { + t.Fatal(err) + } + return buf.Bytes() +} + +func TestDownloadAndExtractSuccess(t *testing.T) { + archive := buildCodeTarGz(t) + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write(archive) + })) + defer srv.Close() + + location := t.TempDir() + if err := downloadAndExtract(srv.URL, location); err != nil { + t.Fatalf("downloadAndExtract: %v", err) + } + if _, err := os.Stat(binaryPath(location)); err != nil { + t.Fatalf("expected extracted code binary: %v", err) + } +} + +func TestDownloadToFileTruncatedTransferReportsDownloadError(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + // Declare more bytes than are written, then return so the connection + // closes early — the client sees a truncated body. + w.Header().Set("Content-Length", "1000") + _, _ = w.Write([]byte("short")) + })) + defer srv.Close() + + err := downloadToFile(srv.URL, &bytes.Buffer{}) + if err == nil { + t.Fatal("expected error for truncated download") + } + if !strings.Contains(err.Error(), "download VS Code CLI") { + t.Fatalf("expected a download-attributed error, got %q", err) + } + if strings.Contains(err.Error(), "extract") { + t.Fatalf("truncation must not be reported as an extract failure: %q", err) + } +} + +func TestDownloadToFileErrorStatus(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusNotFound) + })) + defer srv.Close() + + err := downloadToFile(srv.URL, &bytes.Buffer{}) + if err == nil || !strings.Contains(err.Error(), "404") { + t.Fatalf("expected a 404-attributed error, got %v", err) + } +} + +func TestDownloadAndExtractWithRetryRecoversFromTransientFailure(t *testing.T) { + orig := vscodeCLIBackoff + vscodeCLIBackoff = wait.Backoff{Duration: time.Millisecond, Factor: 1.0, Steps: 3} + defer func() { vscodeCLIBackoff = orig }() + + archive := buildCodeTarGz(t) + var calls int + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + calls++ + if calls == 1 { + w.Header().Set("Content-Length", "1000") + _, _ = w.Write([]byte("short")) + return + } + _, _ = w.Write(archive) + })) + defer srv.Close() + + location := t.TempDir() + if err := downloadAndExtractWithRetry(srv.URL, location); err != nil { + t.Fatalf("expected retry to recover, got %v", err) + } + if calls < 2 { + t.Fatalf("expected at least 2 attempts, got %d", calls) + } + if _, err := os.Stat(filepath.Join(location, "code")); err != nil { + t.Fatalf("expected extracted binary after retry: %v", err) + } +} + func TestGetReleaseURLDefaultVersion(t *testing.T) { v := NewVSCodeWeb(ServerOptions{}) url := v.getReleaseURL() From bba14943acccb3ac61ab9691b0f8129a53deb656 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Wed, 22 Jul 2026 23:46:56 -0500 Subject: [PATCH 02/16] refactor(http): extract shared DownloadToFile helper Introduce devsyhttp.DownloadToFile: a reusable download abstraction with functional options (WithHeaders/WithBackoff/WithClient), context support, transient-vs-permanent error classification (4xx fails fast, 5xx/conn/ truncation retried), Content-Length completeness verification, and fresh truncation of the destination on each retry. Move both duplicated download paths onto it: - vscodeweb.downloadAndExtract now delegates the download (retry + completeness) to the helper and only owns extraction, so an extract failure means a genuinely corrupt archive. - feature.downloadFeatureFromURL drops its hand-rolled retry loop and tryDownload in favor of the helper. Download completeness (Content-Length) lives in the helper; domain integrity (sha256/OCI digest) stays with the caller that knows the expected value. Tests for the helper cover success, headers, truncation retry, transient recovery, 4xx fail-fast, and 5xx retry. --- pkg/devcontainer/feature/features.go | 66 ++---------- pkg/http/download.go | 147 +++++++++++++++++++++++++++ pkg/http/download_test.go | 142 ++++++++++++++++++++++++++ pkg/ide/vscodeweb/vscodeweb.go | 91 ++++------------- pkg/ide/vscodeweb/vscodeweb_test.go | 66 ++---------- 5 files changed, 319 insertions(+), 193 deletions(-) create mode 100644 pkg/http/download.go create mode 100644 pkg/http/download_test.go diff --git a/pkg/devcontainer/feature/features.go b/pkg/devcontainer/feature/features.go index 92c6fec71..1b4070dfd 100644 --- a/pkg/devcontainer/feature/features.go +++ b/pkg/devcontainer/feature/features.go @@ -1,16 +1,15 @@ package feature import ( + "context" "encoding/json" "fmt" "io" - "net/http" "os" "path" "path/filepath" "regexp" "strings" - "time" pkgconfig "github.com/devsy-org/devsy/pkg/config" "github.com/devsy-org/devsy/pkg/devcontainer/config" @@ -546,66 +545,13 @@ func downloadFeatureFromURL( ) error { log.Debugf("starting feature download: url=%s, destFile=%s", url, destFile) - // #nosec G301 -- TODO Consider using a more secure permission setting and ownership if needed. - err := os.MkdirAll(filepath.Dir(destFile), 0o755) - if err != nil { - return fmt.Errorf("create feature folder: %w", err) - } - - attempt := 0 - for range 3 { - if attempt > 0 { - delay := time.Duration(1<= 400 { - return fmt.Errorf("GET request failed, status code is %d", resp.StatusCode) - } - - file, err := os.Create(filepath.Clean(destFile)) //nolint:gosec // path from internal resolution - if err != nil { - return fmt.Errorf("create download file: %w", err) - } - defer func() { _ = file.Close() }() - - _, err = io.Copy(file, resp.Body) - if err != nil { - return fmt.Errorf("download feature: %w", err) + if err := devsyhttp.DownloadToFile( + context.Background(), url, destFile, devsyhttp.WithHeaders(httpHeaders), + ); err != nil { + return err } + log.Infof("Feature download completed successfully: url=%s, destFile=%s", url, destFile) return nil } diff --git a/pkg/http/download.go b/pkg/http/download.go new file mode 100644 index 000000000..538b1a1bb --- /dev/null +++ b/pkg/http/download.go @@ -0,0 +1,147 @@ +package http + +import ( + "context" + "errors" + "fmt" + "io" + "net/http" + "os" + "path/filepath" + "time" + + "github.com/devsy-org/devsy/pkg/log" + "k8s.io/apimachinery/pkg/util/wait" +) + +// DefaultDownloadBackoff retries a download three times with exponential +// backoff, matching the OCI pull retry policy used elsewhere. +var DefaultDownloadBackoff = wait.Backoff{ + Duration: 1 * time.Second, + Factor: 2.0, + Steps: 3, +} + +// downloadConfig holds the resolved options for a download. +type downloadConfig struct { + headers map[string]string + backoff wait.Backoff + client *http.Client +} + +// DownloadOption customizes DownloadToFile. +type DownloadOption func(*downloadConfig) + +// WithHeaders sets request headers sent on every download attempt. +func WithHeaders(headers map[string]string) DownloadOption { + return func(c *downloadConfig) { c.headers = headers } +} + +// WithBackoff overrides the retry backoff policy. +func WithBackoff(backoff wait.Backoff) DownloadOption { + return func(c *downloadConfig) { c.backoff = backoff } +} + +// WithClient overrides the HTTP client used for the download. +func WithClient(client *http.Client) DownloadOption { + return func(c *downloadConfig) { c.client = client } +} + +// permanentError marks a failure that will not be resolved by retrying, such as +// a 4xx response or a malformed request. +type permanentError struct{ err error } + +func (e *permanentError) Error() string { return e.err.Error() } +func (e *permanentError) Unwrap() error { return e.err } + +// DownloadToFile downloads url into destPath, retrying transient failures +// (connection errors, 5xx responses, and truncated transfers) with backoff and +// failing fast on permanent ones (4xx). Each attempt truncates destPath so a +// partial download never leaks into the next attempt, and the transfer is +// verified against Content-Length when the server advertises it — so an +// incomplete download surfaces as a clear error here rather than as a later +// decode failure in the caller. +func DownloadToFile(ctx context.Context, url, destPath string, opts ...DownloadOption) error { + cfg := downloadConfig{backoff: DefaultDownloadBackoff, client: GetHTTPClient()} + for _, opt := range opts { + opt(&cfg) + } + + var lastErr error + err := wait.ExponentialBackoffWithContext( + ctx, + cfg.backoff, + func(ctx context.Context) (bool, error) { + lastErr = downloadOnce(ctx, &cfg, url, destPath) + switch { + case lastErr == nil: + return true, nil + case isPermanent(lastErr): + return false, lastErr + default: + log.Debugf("download attempt failed, retrying: url=%s err=%v", url, lastErr) + return false, nil + } + }, + ) + if wait.Interrupted(err) { + return lastErr + } + return err +} + +func isPermanent(err error) bool { + var perr *permanentError + return errors.As(err, &perr) +} + +func downloadOnce(ctx context.Context, cfg *downloadConfig, url, destPath string) error { + req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil) + if err != nil { + return &permanentError{fmt.Errorf("download %s: %w", url, err)} + } + for key, value := range cfg.headers { + req.Header.Set(key, value) + } + + resp, err := cfg.client.Do(req) + if err != nil { + return fmt.Errorf("download %s: %w", url, err) + } + defer func() { _ = resp.Body.Close() }() + + if resp.StatusCode >= http.StatusBadRequest { + statusErr := fmt.Errorf("download %s: %s", url, resp.Status) + if resp.StatusCode < http.StatusInternalServerError { + return &permanentError{statusErr} + } + return statusErr + } + + return copyToFile(resp, destPath, url) +} + +func copyToFile(resp *http.Response, destPath, url string) error { + // #nosec G301 -- match existing download destinations; contents are public artifacts. + if err := os.MkdirAll(filepath.Dir(destPath), 0o755); err != nil { + return &permanentError{fmt.Errorf("create download folder: %w", err)} + } + + file, err := os.OpenFile(filepath.Clean(destPath), os.O_CREATE|os.O_WRONLY|os.O_TRUNC, 0o600) + if err != nil { + return &permanentError{fmt.Errorf("create download file: %w", err)} + } + defer func() { _ = file.Close() }() + + written, err := io.Copy(file, resp.Body) + if err != nil { + return fmt.Errorf("download %s: %w", url, err) + } + if resp.ContentLength >= 0 && written != resp.ContentLength { + return fmt.Errorf( + "download %s: incomplete transfer, got %d of %d bytes", + url, written, resp.ContentLength, + ) + } + return nil +} diff --git a/pkg/http/download_test.go b/pkg/http/download_test.go new file mode 100644 index 000000000..fea79aa16 --- /dev/null +++ b/pkg/http/download_test.go @@ -0,0 +1,142 @@ +package http + +import ( + "context" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "strings" + "testing" + "time" + + "k8s.io/apimachinery/pkg/util/wait" +) + +func fastBackoff() DownloadOption { + return WithBackoff(wait.Backoff{Duration: time.Millisecond, Factor: 1.0, Steps: 3}) +} + +func TestDownloadToFileSuccess(t *testing.T) { + const body = "hello-payload" + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte(body)) + })) + defer srv.Close() + + dest := filepath.Join(t.TempDir(), "out.bin") + if err := DownloadToFile(context.Background(), srv.URL, dest, fastBackoff()); err != nil { + t.Fatalf("DownloadToFile: %v", err) + } + got, err := os.ReadFile(dest) // #nosec G304 -- test-controlled temp path + if err != nil { + t.Fatal(err) + } + if string(got) != body { + t.Fatalf("content mismatch: got %q want %q", got, body) + } +} + +func TestDownloadToFileSendsHeaders(t *testing.T) { + var gotHeader string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + gotHeader = r.Header.Get("Foo-Header") + _, _ = w.Write([]byte("ok")) + })) + defer srv.Close() + + dest := filepath.Join(t.TempDir(), "out.bin") + err := DownloadToFile(context.Background(), srv.URL, dest, + WithHeaders(map[string]string{"Foo-Header": "Bar"}), fastBackoff()) + if err != nil { + t.Fatalf("DownloadToFile: %v", err) + } + if gotHeader != "Bar" { + t.Fatalf("expected header to be sent, got %q", gotHeader) + } +} + +func TestDownloadToFileTruncatedRetriesThenReportsClearError(t *testing.T) { + var calls int + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + calls++ + w.Header().Set("Content-Length", "1000") + _, _ = w.Write([]byte("short")) + })) + defer srv.Close() + + dest := filepath.Join(t.TempDir(), "out.bin") + err := DownloadToFile(context.Background(), srv.URL, dest, fastBackoff()) + if err == nil { + t.Fatal("expected error for repeated truncated transfers") + } + if !strings.Contains(err.Error(), "download") { + t.Fatalf("expected a download-attributed error, got %q", err) + } + if calls != 3 { + t.Fatalf("expected 3 attempts (transient), got %d", calls) + } +} + +func TestDownloadToFileRecoversAfterTransientFailure(t *testing.T) { + const body = "recovered-payload" + var calls int + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + calls++ + if calls == 1 { + w.Header().Set("Content-Length", "1000") + _, _ = w.Write([]byte("short")) + return + } + _, _ = w.Write([]byte(body)) + })) + defer srv.Close() + + dest := filepath.Join(t.TempDir(), "out.bin") + if err := DownloadToFile(context.Background(), srv.URL, dest, fastBackoff()); err != nil { + t.Fatalf("expected recovery, got %v", err) + } + got, err := os.ReadFile(dest) // #nosec G304 -- test-controlled temp path + if err != nil { + t.Fatal(err) + } + if string(got) != body { + t.Fatalf("expected fresh content after retry, got %q", got) + } +} + +func TestDownloadToFilePermanentStatusFailsFast(t *testing.T) { + var calls int + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + calls++ + w.WriteHeader(http.StatusNotFound) + })) + defer srv.Close() + + dest := filepath.Join(t.TempDir(), "out.bin") + err := DownloadToFile(context.Background(), srv.URL, dest, fastBackoff()) + if err == nil || !strings.Contains(err.Error(), "404") { + t.Fatalf("expected a 404 error, got %v", err) + } + if calls != 1 { + t.Fatalf("expected 4xx to fail fast without retry, got %d attempts", calls) + } +} + +func TestDownloadToFileServerErrorRetries(t *testing.T) { + var calls int + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + calls++ + w.WriteHeader(http.StatusInternalServerError) + })) + defer srv.Close() + + dest := filepath.Join(t.TempDir(), "out.bin") + err := DownloadToFile(context.Background(), srv.URL, dest, fastBackoff()) + if err == nil { + t.Fatal("expected error for persistent 5xx") + } + if calls != 3 { + t.Fatalf("expected 5xx to be retried, got %d attempts", calls) + } +} diff --git a/pkg/ide/vscodeweb/vscodeweb.go b/pkg/ide/vscodeweb/vscodeweb.go index a4a56fe42..9ca2b9f0a 100644 --- a/pkg/ide/vscodeweb/vscodeweb.go +++ b/pkg/ide/vscodeweb/vscodeweb.go @@ -1,15 +1,13 @@ package vscodeweb import ( + "context" "fmt" - "io" - "net/http" "os" "os/exec" "path/filepath" "runtime" "strconv" - "time" "github.com/devsy-org/devsy/pkg/command" "github.com/devsy-org/devsy/pkg/config" @@ -20,7 +18,6 @@ import ( "github.com/devsy-org/devsy/pkg/ide/vscode" "github.com/devsy-org/devsy/pkg/log" "github.com/devsy-org/devsy/pkg/util" - "k8s.io/apimachinery/pkg/util/wait" ) // Release URL format for the standalone VS Code CLI. The CLI tarball contains a @@ -129,7 +126,7 @@ func (v *VSCodeWeb) Install() error { if !v.isInstalled(location, releaseURL) { vscode.InstallAPKRequirements() - if err := downloadAndExtractWithRetry(releaseURL, location); err != nil { + if err := downloadAndExtract(releaseURL, location); err != nil { return err } @@ -299,60 +296,33 @@ func (v *VSCodeWeb) installSettings() error { return nil } -var vscodeCLIBackoff = wait.Backoff{ - Duration: 1 * time.Second, - Factor: 2.0, - Steps: 3, -} - -// downloadAndExtractWithRetry retries the CLI download and extraction, backing -// off between attempts. Failures here are dominated by transient transfer -// truncation (the CLI is served from a CDN), so a single hiccup should not fail -// the whole up. downloadAndExtract cleans up a partial install on failure, so -// each attempt starts fresh. -func downloadAndExtractWithRetry(url, location string) error { - var lastErr error - err := wait.ExponentialBackoff(vscodeCLIBackoff, func() (bool, error) { - lastErr = downloadAndExtract(url, location) - if lastErr == nil { - return true, nil - } - log.Warnf("VS Code CLI install failed, retrying: %v", lastErr) - return false, nil - }) - if wait.Interrupted(err) { - return lastErr - } - return err -} - -// downloadAndExtract downloads the CLI tarball at url to a temporary file, -// verifies the transfer completed, then extracts it under location. Downloading -// to a file first (rather than streaming the response body into the extractor) -// lets a truncated transfer surface as a clear, retryable error instead of an -// opaque decompression EOF. The VS Code CLI tarball holds a single `code` -// binary at its root, so no strip levels are applied. Cleans up a partial -// extraction on failure so retries start fresh. +// downloadAndExtract downloads the CLI tarball at url to a temporary file, then +// extracts it under location. The download (with retry and transfer-completeness +// verification) is delegated to devsyhttp.DownloadToFile, so a truncated +// transfer is retried and reported as a clear download error; an extract failure +// here therefore means a genuinely corrupt archive. The VS Code CLI tarball +// holds a single `code` binary at its root, so no strip levels are applied. +// Cleans up a partial extraction on failure so retries start fresh. func downloadAndExtract(url, location string) error { tmpFile, err := os.CreateTemp("", "vscode-cli-*.tar.gz") if err != nil { return fmt.Errorf("create temp file for VS Code CLI: %w", err) } tmpPath := tmpFile.Name() - defer func() { - _ = tmpFile.Close() - _ = os.Remove(tmpPath) - }() + _ = tmpFile.Close() + defer func() { _ = os.Remove(tmpPath) }() - if err := downloadToFile(url, tmpFile); err != nil { + if err := devsyhttp.DownloadToFile(context.Background(), url, tmpPath); err != nil { return err } - if _, err := tmpFile.Seek(0, io.SeekStart); err != nil { - return fmt.Errorf("rewind VS Code CLI download: %w", err) + file, err := os.Open(filepath.Clean(tmpPath)) + if err != nil { + return err } + defer func() { _ = file.Close() }() - if err := extract.Extract(tmpFile, location); err != nil { + if err := extract.Extract(file, location); err != nil { if rmErr := os.RemoveAll(location); rmErr != nil { log.Warnf("cleanup partial install: path=%s err=%v", location, rmErr) } @@ -361,33 +331,6 @@ func downloadAndExtract(url, location string) error { return nil } -// downloadToFile downloads url into dst, rejecting error statuses and short -// transfers so an incomplete download is reported clearly rather than as a -// later decompression failure. -func downloadToFile(url string, dst io.Writer) error { - resp, err := devsyhttp.GetHTTPClient().Get(url) // #nosec G107 -- URL comes from VersionOption. - if err != nil { - return fmt.Errorf("download VS Code CLI: %w", err) - } - defer func() { _ = resp.Body.Close() }() - - if resp.StatusCode >= http.StatusBadRequest { - return fmt.Errorf("download VS Code CLI: %s returned %s", url, resp.Status) - } - - written, err := io.Copy(dst, resp.Body) - if err != nil { - return fmt.Errorf("download VS Code CLI: %w", err) - } - if resp.ContentLength >= 0 && written != resp.ContentLength { - return fmt.Errorf( - "download VS Code CLI: incomplete transfer, got %d of %d bytes", - written, resp.ContentLength, - ) - } - return nil -} - // suOrSh builds an *exec.Cmd that runs runCommand either as the unprivileged // user (via `su -c`) or as the current user (via `sh -c`). All arg // elements are constants from this package — runCommand itself is built from diff --git a/pkg/ide/vscodeweb/vscodeweb_test.go b/pkg/ide/vscodeweb/vscodeweb_test.go index c26321093..86f6fc9e1 100644 --- a/pkg/ide/vscodeweb/vscodeweb_test.go +++ b/pkg/ide/vscodeweb/vscodeweb_test.go @@ -7,14 +7,11 @@ import ( "net/http" "net/http/httptest" "os" - "path/filepath" "runtime" "strings" "testing" - "time" "github.com/devsy-org/devsy/pkg/config" - "k8s.io/apimachinery/pkg/util/wait" ) // buildCodeTarGz returns a gzipped tar holding a single `code` binary at its @@ -58,66 +55,17 @@ func TestDownloadAndExtractSuccess(t *testing.T) { } } -func TestDownloadToFileTruncatedTransferReportsDownloadError(t *testing.T) { +func TestDownloadAndExtractCorruptArchiveReportsExtractError(t *testing.T) { srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { - // Declare more bytes than are written, then return so the connection - // closes early — the client sees a truncated body. - w.Header().Set("Content-Length", "1000") - _, _ = w.Write([]byte("short")) + // A complete transfer of bytes that are not a valid archive: the + // download succeeds, so the failure must be attributed to extraction. + _, _ = w.Write([]byte("not-a-tarball")) })) defer srv.Close() - err := downloadToFile(srv.URL, &bytes.Buffer{}) - if err == nil { - t.Fatal("expected error for truncated download") - } - if !strings.Contains(err.Error(), "download VS Code CLI") { - t.Fatalf("expected a download-attributed error, got %q", err) - } - if strings.Contains(err.Error(), "extract") { - t.Fatalf("truncation must not be reported as an extract failure: %q", err) - } -} - -func TestDownloadToFileErrorStatus(t *testing.T) { - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { - w.WriteHeader(http.StatusNotFound) - })) - defer srv.Close() - - err := downloadToFile(srv.URL, &bytes.Buffer{}) - if err == nil || !strings.Contains(err.Error(), "404") { - t.Fatalf("expected a 404-attributed error, got %v", err) - } -} - -func TestDownloadAndExtractWithRetryRecoversFromTransientFailure(t *testing.T) { - orig := vscodeCLIBackoff - vscodeCLIBackoff = wait.Backoff{Duration: time.Millisecond, Factor: 1.0, Steps: 3} - defer func() { vscodeCLIBackoff = orig }() - - archive := buildCodeTarGz(t) - var calls int - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { - calls++ - if calls == 1 { - w.Header().Set("Content-Length", "1000") - _, _ = w.Write([]byte("short")) - return - } - _, _ = w.Write(archive) - })) - defer srv.Close() - - location := t.TempDir() - if err := downloadAndExtractWithRetry(srv.URL, location); err != nil { - t.Fatalf("expected retry to recover, got %v", err) - } - if calls < 2 { - t.Fatalf("expected at least 2 attempts, got %d", calls) - } - if _, err := os.Stat(filepath.Join(location, "code")); err != nil { - t.Fatalf("expected extracted binary after retry: %v", err) + err := downloadAndExtract(srv.URL, t.TempDir()) + if err == nil || !strings.Contains(err.Error(), "extract VS Code CLI") { + t.Fatalf("expected an extract-attributed error, got %v", err) } } From bc6ae5f61950e8c2a8322f94eda1196c75860acd Mon Sep 17 00:00:00 2001 From: Samuel K Date: Wed, 22 Jul 2026 23:53:03 -0500 Subject: [PATCH 03/16] fix(http): verify TLS by default for the shared HTTP client MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The shared HTTP client set InsecureSkipVerify unconditionally, disabling TLS certificate verification for every caller — including IDE/agent binary downloads from public CDNs, exposing them to MITM. Genuinely-insecure paths (self-signed pro/platform instances, in-container setup) already use their own dedicated insecure transports, and the localhost credential flows are plain HTTP, so the shared client does not need it. Verify certificates by default and disable only via the DEVSY_INSECURE_TLS opt-in escape hatch for environments that must reach self-signed hosts. --- pkg/config/env.go | 6 ++++++ pkg/http/http.go | 20 +++++++++++++++++++- pkg/http/http_test.go | 30 ++++++++++++++++++++++++++++++ 3 files changed, 55 insertions(+), 1 deletion(-) create mode 100644 pkg/http/http_test.go diff --git a/pkg/config/env.go b/pkg/config/env.go index 50fa53c25..dd3185cc1 100644 --- a/pkg/config/env.go +++ b/pkg/config/env.go @@ -25,6 +25,12 @@ const ( // EnvDebug enables debug logging. EnvDebug = "DEVSY_DEBUG" + // EnvInsecureTLS, when truthy, disables TLS certificate verification for the + // shared HTTP client. It exists only as an escape hatch for environments + // that must reach hosts with self-signed certificates; verification is on by + // default. + EnvInsecureTLS = "DEVSY_INSECURE_TLS" + // EnvDisableTelemetry disables telemetry collection. EnvDisableTelemetry = "DEVSY_DISABLE_TELEMETRY" diff --git a/pkg/http/http.go b/pkg/http/http.go index c3deda938..814384b5c 100644 --- a/pkg/http/http.go +++ b/pkg/http/http.go @@ -3,7 +3,12 @@ package http import ( "crypto/tls" "net/http" + "os" + "strconv" "sync" + + "github.com/devsy-org/devsy/pkg/config" + "github.com/devsy-org/devsy/pkg/log" ) var ( @@ -14,9 +19,22 @@ var ( func GetHTTPClient() *http.Client { httpClientOnce.Do(func() { customTransport := http.DefaultTransport.(*http.Transport).Clone() - customTransport.TLSClientConfig = &tls.Config{InsecureSkipVerify: true} + if insecureTLSEnabled() { + // #nosec G402 -- opt-in escape hatch (DEVSY_INSECURE_TLS) for + // environments that must reach hosts with self-signed certificates. + customTransport.TLSClientConfig = &tls.Config{InsecureSkipVerify: true} + log.Warnf("TLS certificate verification is disabled (%s)", config.EnvInsecureTLS) + } httpClient = &http.Client{Transport: customTransport} }) return httpClient } + +// insecureTLSEnabled reports whether TLS certificate verification should be +// disabled for the shared HTTP client. Verification is on by default and is +// only disabled when EnvInsecureTLS is set to a truthy value. +func insecureTLSEnabled() bool { + enabled, _ := strconv.ParseBool(os.Getenv(config.EnvInsecureTLS)) + return enabled +} diff --git a/pkg/http/http_test.go b/pkg/http/http_test.go new file mode 100644 index 000000000..54d17ca20 --- /dev/null +++ b/pkg/http/http_test.go @@ -0,0 +1,30 @@ +package http + +import ( + "testing" + + "github.com/devsy-org/devsy/pkg/config" +) + +func TestInsecureTLSEnabled(t *testing.T) { + tests := []struct { + name string + value string + want bool + }{ + {name: "unset defaults to secure", value: "", want: false}, + {name: "true enables insecure", value: "true", want: true}, + {name: "1 enables insecure", value: "1", want: true}, + {name: "false stays secure", value: "false", want: false}, + {name: "garbage stays secure", value: "yes-please", want: false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Setenv(config.EnvInsecureTLS, tt.value) + if got := insecureTLSEnabled(); got != tt.want { + t.Errorf("insecureTLSEnabled() with %q = %v, want %v", tt.value, got, tt.want) + } + }) + } +} From d6028a228118c93431cbb8541b2eee82dc0c3064 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Thu, 23 Jul 2026 00:06:59 -0500 Subject: [PATCH 04/16] refactor(ide): route all IDE binary downloads through DownloadToFile Migrate every download-to-file site onto the shared helper, giving each the retry + completeness verification it lacked: - openvscode, codeserver: previously streamed the response body straight into the extractor (same truncation-EOF bug as vscode). Now download to a temp file first, then extract. - fleet: download the binary via the helper with WithMode(0o755). - rstudio, jetbrains: download the archive via the helper with SkipIfSameSize (preserving the existing size-based cache reuse) and WithProgress (preserving the progress log), dropping their bespoke fetch/save helpers. Extends DownloadToFile with WithMode, WithProgress, and SkipIfSameSize options. Left alone (not download-to-file): pkg/download (stream-returning API with GitHub private-release auth), pkg/agent/binary (streaming tee-to-cache), and the API/version/ping/POST callers. --- pkg/http/download.go | 60 +++++++++++++++++++++++++++----- pkg/http/download_test.go | 55 +++++++++++++++++++++++++++++ pkg/ide/codeserver/codeserver.go | 25 ++++++++----- pkg/ide/fleet/fleet.go | 26 +++----------- pkg/ide/jetbrains/generic.go | 49 +++++--------------------- pkg/ide/openvscode/openvscode.go | 55 +++++++++++++++++++++-------- pkg/ide/rstudio/rstudio.go | 57 ++++++------------------------ 7 files changed, 187 insertions(+), 140 deletions(-) diff --git a/pkg/http/download.go b/pkg/http/download.go index 538b1a1bb..a02962d36 100644 --- a/pkg/http/download.go +++ b/pkg/http/download.go @@ -24,9 +24,12 @@ var DefaultDownloadBackoff = wait.Backoff{ // downloadConfig holds the resolved options for a download. type downloadConfig struct { - headers map[string]string - backoff wait.Backoff - client *http.Client + headers map[string]string + backoff wait.Backoff + client *http.Client + mode os.FileMode + wrap func(r io.Reader, totalSize int64) io.Reader + skipIfSized bool } // DownloadOption customizes DownloadToFile. @@ -47,6 +50,25 @@ func WithClient(client *http.Client) DownloadOption { return func(c *downloadConfig) { c.client = client } } +// WithMode sets the permission bits for the destination file (default 0o600), +// for downloads that must be executable. +func WithMode(mode os.FileMode) DownloadOption { + return func(c *downloadConfig) { c.mode = mode } +} + +// WithProgress wraps the response body on each attempt, e.g. to report download +// progress. wrap receives the body reader and the advertised size (-1 when +// unknown) and returns the reader to copy from. +func WithProgress(wrap func(r io.Reader, totalSize int64) io.Reader) DownloadOption { + return func(c *downloadConfig) { c.wrap = wrap } +} + +// SkipIfSameSize skips the download when destPath already exists and its size +// matches the advertised Content-Length, reusing the cached file. +func SkipIfSameSize() DownloadOption { + return func(c *downloadConfig) { c.skipIfSized = true } +} + // permanentError marks a failure that will not be resolved by retrying, such as // a 4xx response or a malformed request. type permanentError struct{ err error } @@ -62,7 +84,7 @@ func (e *permanentError) Unwrap() error { return e.err } // incomplete download surfaces as a clear error here rather than as a later // decode failure in the caller. func DownloadToFile(ctx context.Context, url, destPath string, opts ...DownloadOption) error { - cfg := downloadConfig{backoff: DefaultDownloadBackoff, client: GetHTTPClient()} + cfg := downloadConfig{backoff: DefaultDownloadBackoff, client: GetHTTPClient(), mode: 0o600} for _, opt := range opts { opt(&cfg) } @@ -118,25 +140,47 @@ func downloadOnce(ctx context.Context, cfg *downloadConfig, url, destPath string return statusErr } - return copyToFile(resp, destPath, url) + return copyToFile(resp, cfg, destPath, url) } -func copyToFile(resp *http.Response, destPath, url string) error { +func copyToFile(resp *http.Response, cfg *downloadConfig, destPath, url string) error { + if alreadyDownloaded(cfg, resp, destPath) { + return nil + } + // #nosec G301 -- match existing download destinations; contents are public artifacts. if err := os.MkdirAll(filepath.Dir(destPath), 0o755); err != nil { return &permanentError{fmt.Errorf("create download folder: %w", err)} } - file, err := os.OpenFile(filepath.Clean(destPath), os.O_CREATE|os.O_WRONLY|os.O_TRUNC, 0o600) + file, err := os.OpenFile(filepath.Clean(destPath), os.O_CREATE|os.O_WRONLY|os.O_TRUNC, cfg.mode) if err != nil { return &permanentError{fmt.Errorf("create download file: %w", err)} } defer func() { _ = file.Close() }() - written, err := io.Copy(file, resp.Body) + src := io.Reader(resp.Body) + if cfg.wrap != nil { + src = cfg.wrap(resp.Body, resp.ContentLength) + } + written, err := io.Copy(file, src) if err != nil { return fmt.Errorf("download %s: %w", url, err) } + return verifyContentLength(resp, written, url) +} + +// alreadyDownloaded reports whether destPath already holds the full download, +// based on a size match against the advertised Content-Length. +func alreadyDownloaded(cfg *downloadConfig, resp *http.Response, destPath string) bool { + if !cfg.skipIfSized || resp.ContentLength < 0 { + return false + } + info, err := os.Stat(filepath.Clean(destPath)) + return err == nil && info.Size() == resp.ContentLength +} + +func verifyContentLength(resp *http.Response, written int64, url string) error { if resp.ContentLength >= 0 && written != resp.ContentLength { return fmt.Errorf( "download %s: incomplete transfer, got %d of %d bytes", diff --git a/pkg/http/download_test.go b/pkg/http/download_test.go index fea79aa16..aaae8eabb 100644 --- a/pkg/http/download_test.go +++ b/pkg/http/download_test.go @@ -2,10 +2,12 @@ package http import ( "context" + "io" "net/http" "net/http/httptest" "os" "path/filepath" + "strconv" "strings" "testing" "time" @@ -123,6 +125,59 @@ func TestDownloadToFilePermanentStatusFailsFast(t *testing.T) { } } +func TestDownloadToFileWithMode(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte("binary")) + })) + defer srv.Close() + + dest := filepath.Join(t.TempDir(), "bin") + if err := DownloadToFile( + context.Background(), + srv.URL, + dest, + WithMode(0o755), + fastBackoff(), + ); err != nil { + t.Fatalf("DownloadToFile: %v", err) + } + info, err := os.Stat(dest) + if err != nil { + t.Fatal(err) + } + if info.Mode().Perm()&0o100 == 0 { + t.Fatalf("expected owner-executable file, got mode %v", info.Mode().Perm()) + } +} + +func TestDownloadToFileSkipIfSameSize(t *testing.T) { + const body = "cached-bytes" + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + // Same length as the pre-existing file, but different content. + w.Header().Set("Content-Length", strconv.Itoa(len(body))) + _, _ = io.WriteString(w, strings.Repeat("x", len(body))) + })) + defer srv.Close() + + dest := filepath.Join(t.TempDir(), "out.bin") + if err := os.WriteFile(dest, []byte(body), 0o600); err != nil { + t.Fatal(err) + } + + if err := DownloadToFile( + context.Background(), srv.URL, dest, SkipIfSameSize(), fastBackoff(), + ); err != nil { + t.Fatalf("DownloadToFile: %v", err) + } + got, err := os.ReadFile(dest) // #nosec G304 -- test-controlled temp path + if err != nil { + t.Fatal(err) + } + if string(got) != body { + t.Fatalf("expected existing file preserved on size match, got %q", got) + } +} + func TestDownloadToFileServerErrorRetries(t *testing.T) { var calls int srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { diff --git a/pkg/ide/codeserver/codeserver.go b/pkg/ide/codeserver/codeserver.go index 0f4070b74..909562bea 100644 --- a/pkg/ide/codeserver/codeserver.go +++ b/pkg/ide/codeserver/codeserver.go @@ -1,8 +1,8 @@ package codeserver import ( + "context" "fmt" - "net/http" "os" "os/exec" "path/filepath" @@ -271,20 +271,29 @@ func (c *CodeServer) installSettings() error { return nil } -// downloadAndExtract fetches the release tarball at url and extracts it under -// location. Cleans up a partial extraction on failure so retries start fresh. +// downloadAndExtract downloads the release tarball at url to a temporary file +// (with retry and completeness verification) then extracts it under location. +// Cleans up a partial extraction on failure so retries start fresh. func downloadAndExtract(url, location string) error { - resp, err := devsyhttp.GetHTTPClient().Get(url) // #nosec G107 -- URL comes from VersionOption. + tmpFile, err := os.CreateTemp("", "code-server-*.tar.gz") if err != nil { + return fmt.Errorf("create temp file: %w", err) + } + tmpPath := tmpFile.Name() + _ = tmpFile.Close() + defer func() { _ = os.Remove(tmpPath) }() + + if err := devsyhttp.DownloadToFile(context.Background(), url, tmpPath); err != nil { return err } - defer func() { _ = resp.Body.Close() }() - if resp.StatusCode >= http.StatusBadRequest { - return fmt.Errorf("download code-server: %s returned %s", url, resp.Status) + file, err := os.Open(filepath.Clean(tmpPath)) + if err != nil { + return err } + defer func() { _ = file.Close() }() - if err := extract.Extract(resp.Body, location, extract.StripLevels(1)); err != nil { + if err := extract.Extract(file, location, extract.StripLevels(1)); err != nil { if rmErr := os.RemoveAll(location); rmErr != nil { log.Warnf("cleanup partial install: path=%s err=%v", location, rmErr) } diff --git a/pkg/ide/fleet/fleet.go b/pkg/ide/fleet/fleet.go index 950031952..b19cc8067 100644 --- a/pkg/ide/fleet/fleet.go +++ b/pkg/ide/fleet/fleet.go @@ -2,6 +2,7 @@ package fleet import ( "bytes" + "context" "fmt" "io" "os" @@ -83,30 +84,11 @@ func (o *FleetServer) Install(projectDir string) error { // download binary log.Infof("Downloading fleet") - resp, err := devsyhttp.GetHTTPClient().Get(url) - if err != nil { + if err := devsyhttp.DownloadToFile( + context.Background(), url, fleetBinary, devsyhttp.WithMode(0o755), + ); err != nil { return err } - defer func() { _ = resp.Body.Close() }() - if resp.StatusCode != 200 { - return fmt.Errorf( - "unexpected status code while trying to download fleet from %s: %d", - url, - resp.StatusCode, - ) - } - - f, err := os.OpenFile(fleetBinary, os.O_RDWR|os.O_CREATE|os.O_TRUNC, 0o755) // #nosec G302,G304 - if err != nil { - return err - } - defer func() { _ = f.Close() }() - - _, err = io.Copy(f, resp.Body) - if err != nil { - return fmt.Errorf("download fleet: %w", err) - } - _ = f.Close() // chown location if o.userName != "" { diff --git a/pkg/ide/jetbrains/generic.go b/pkg/ide/jetbrains/generic.go index 34b976125..3f84bffbe 100644 --- a/pkg/ide/jetbrains/generic.go +++ b/pkg/ide/jetbrains/generic.go @@ -1,9 +1,9 @@ package jetbrains import ( + "context" "fmt" "io" - "net/http" "net/url" "os" "os/exec" @@ -244,48 +244,15 @@ func (o *GenericJetBrainsServer) download(targetFolder string) (string, error) { o.options.ID, ) - resp, err := fetchArchive(downloadURL) - if err != nil { - return "", err - } - defer func() { _ = resp.Body.Close() }() - - return saveArchive(targetPath, resp) -} - -func fetchArchive(downloadURL string) (*http.Response, error) { - resp, err := devsyhttp.GetHTTPClient().Get(downloadURL) - if err != nil { - return nil, fmt.Errorf("download binary: %w", err) - } - - if resp.StatusCode != http.StatusOK && resp.StatusCode != http.StatusCreated { - _ = resp.Body.Close() - return nil, fmt.Errorf("download binary returned status code %d: %w", resp.StatusCode, err) - } - - return resp, nil -} - -func saveArchive(targetPath string, resp *http.Response) (string, error) { - stat, err := os.Stat(targetPath) - if err == nil && stat.Size() == resp.ContentLength { - return targetPath, nil - } - - file, err := os.Create(targetPath) - if err != nil { + if err := devsyhttp.DownloadToFile( + context.Background(), downloadURL, targetPath, + devsyhttp.SkipIfSameSize(), + devsyhttp.WithProgress(func(r io.Reader, totalSize int64) io.Reader { + return &ide.ProgressReader{Reader: r, TotalSize: totalSize} + }), + ); err != nil { return "", err } - defer func() { _ = file.Close() }() - - _, err = io.Copy(file, &ide.ProgressReader{ - Reader: resp.Body, - TotalSize: resp.ContentLength, - }) - if err != nil { - return "", fmt.Errorf("download file: %w", err) - } return targetPath, nil } diff --git a/pkg/ide/openvscode/openvscode.go b/pkg/ide/openvscode/openvscode.go index d204aee02..965180f8f 100644 --- a/pkg/ide/openvscode/openvscode.go +++ b/pkg/ide/openvscode/openvscode.go @@ -1,6 +1,7 @@ package openvscode import ( + "context" "fmt" "os" "os/exec" @@ -118,31 +119,57 @@ func (o *OpenVSCodeServer) Install() error { vscode.InstallAPKRequirements() // download tar - resp, err := devsyhttp.GetHTTPClient().Get(url) - if err != nil { + if err := downloadAndExtract(url, location); err != nil { return err } - defer func() { _ = resp.Body.Close() }() - err = extract.Extract(resp.Body, location, extract.StripLevels(1)) + // chown location + if err := chownIfNeeded(o.userName, location); err != nil { + return err + } + + // paste settings + if err := o.installSettings(); err != nil { + return fmt.Errorf("install settings: %w", err) + } + + return nil +} + +// downloadAndExtract downloads the openvscode-server tarball to a temporary file +// (with retry and completeness verification) then extracts it under location. +func downloadAndExtract(url, location string) error { + tmpFile, err := os.CreateTemp("", "openvscode-*.tar.gz") if err != nil { - return fmt.Errorf("extract vscode: %w", err) + return fmt.Errorf("create temp file: %w", err) } + tmpPath := tmpFile.Name() + _ = tmpFile.Close() + defer func() { _ = os.Remove(tmpPath) }() - // chown location - if o.userName != "" { - err = copy2.ChownR(location, o.userName) - if err != nil { - return fmt.Errorf("chown: %w", err) - } + if err := devsyhttp.DownloadToFile(context.Background(), url, tmpPath); err != nil { + return err } - // paste settings - err = o.installSettings() + file, err := os.Open(filepath.Clean(tmpPath)) if err != nil { - return fmt.Errorf("install settings: %w", err) + return err + } + defer func() { _ = file.Close() }() + + if err := extract.Extract(file, location, extract.StripLevels(1)); err != nil { + return fmt.Errorf("extract vscode: %w", err) } + return nil +} +func chownIfNeeded(userName, location string) error { + if userName == "" { + return nil + } + if err := copy2.ChownR(location, userName); err != nil { + return fmt.Errorf("chown: %w", err) + } return nil } diff --git a/pkg/ide/rstudio/rstudio.go b/pkg/ide/rstudio/rstudio.go index b27614647..595292ace 100644 --- a/pkg/ide/rstudio/rstudio.go +++ b/pkg/ide/rstudio/rstudio.go @@ -2,11 +2,11 @@ package rstudio import ( "bytes" + "context" _ "embed" "encoding/json" "fmt" "io" - "net/http" "os" "os/exec" "path/filepath" @@ -215,63 +215,26 @@ func getDownloadURL(version, ubuntuCodename, architecture string) string { } func download(targetFolder, downloadURL string) (string, error) { - err := os.MkdirAll(targetFolder, os.ModePerm) - if err != nil { + // #nosec G301 -- IDE install dir; matches other IDE download destinations. + if err := os.MkdirAll(targetFolder, 0o755); err != nil { return "", err } targetPath := filepath.Join(filepath.ToSlash(targetFolder), "rstudio-server.deb") - resp, err := devsyhttp.GetHTTPClient().Get(downloadURL) - if err != nil { - return "", fmt.Errorf("download deb: %w", err) - } - defer func() { _ = resp.Body.Close() }() - - if err := validateResponseStatus(resp, downloadURL); err != nil { - return "", err - } - - stat, err := os.Stat(targetPath) - if err == nil && stat.Size() == resp.ContentLength { - return targetPath, nil - } - - if err := saveResponseToFile(targetPath, resp); err != nil { + if err := devsyhttp.DownloadToFile( + context.Background(), downloadURL, targetPath, + devsyhttp.SkipIfSameSize(), + devsyhttp.WithProgress(func(r io.Reader, totalSize int64) io.Reader { + return &ide.ProgressReader{Reader: r, TotalSize: totalSize} + }), + ); err != nil { return "", err } return targetPath, nil } -func validateResponseStatus(resp *http.Response, downloadURL string) error { - if resp.StatusCode == http.StatusOK || resp.StatusCode == http.StatusCreated { - return nil - } - if resp.StatusCode == http.StatusNotFound { - return fmt.Errorf("RStudio version doesn't exist: %s", downloadURL) //nolint:all - } - return fmt.Errorf("download binary returned status code %d", resp.StatusCode) -} - -func saveResponseToFile(targetPath string, resp *http.Response) error { - file, err := os.Create(targetPath) - if err != nil { - return err - } - defer func() { _ = file.Close() }() - - _, err = io.Copy(file, &ide.ProgressReader{ - Reader: resp.Body, - TotalSize: resp.ContentLength, - }) - if err != nil { - return fmt.Errorf("download file: %w", err) - } - - return nil -} - func installDeb(debPath string) error { log.Info("Installing deb") From 7482176e3b780bf9db516a4ce4dd9968ab55aa8b Mon Sep 17 00:00:00 2001 From: Samuel K Date: Thu, 23 Jul 2026 00:17:26 -0500 Subject: [PATCH 05/16] refactor(ide): add ide.DownloadAndExtract for download+extract composition The download-to-temp-file then extract dance was duplicated across vscodeweb, openvscode, and codeserver. Extract it into ide.DownloadAndExtract, which composes devsyhttp.DownloadToFile (transport + retry + completeness) with extract.Extract (archive), passing through extract options (e.g. StripLevels) and cleaning up a partial destination on failure. Deliberately placed in pkg/ide rather than the http client: composing HTTP transport with archive extraction belongs in the layer above both, keeping pkg/http and pkg/extract focused (neither imports the other or pkg/ide). All three IDE installers already depend on pkg/ide. Tests move with the logic into pkg/ide (success, corrupt-archive attribution, cleanup on failure). --- pkg/ide/codeserver/codeserver.go | 36 ++----------- pkg/ide/download.go | 51 ++++++++++++++++++ pkg/ide/download_test.go | 82 +++++++++++++++++++++++++++++ pkg/ide/openvscode/openvscode.go | 32 ++--------- pkg/ide/vscodeweb/vscodeweb.go | 39 +------------- pkg/ide/vscodeweb/vscodeweb_test.go | 60 --------------------- 6 files changed, 140 insertions(+), 160 deletions(-) create mode 100644 pkg/ide/download.go create mode 100644 pkg/ide/download_test.go diff --git a/pkg/ide/codeserver/codeserver.go b/pkg/ide/codeserver/codeserver.go index 909562bea..e90955b39 100644 --- a/pkg/ide/codeserver/codeserver.go +++ b/pkg/ide/codeserver/codeserver.go @@ -13,7 +13,6 @@ import ( "github.com/devsy-org/devsy/pkg/config" copy2 "github.com/devsy-org/devsy/pkg/copy" "github.com/devsy-org/devsy/pkg/extract" - devsyhttp "github.com/devsy-org/devsy/pkg/http" "github.com/devsy-org/devsy/pkg/ide" "github.com/devsy-org/devsy/pkg/ide/vscode" "github.com/devsy-org/devsy/pkg/log" @@ -121,7 +120,9 @@ func (c *CodeServer) Install() error { vscode.InstallAPKRequirements() - if err := downloadAndExtract(c.getReleaseURL(), location); err != nil { + if err := ide.DownloadAndExtract( + context.Background(), c.getReleaseURL(), location, extract.StripLevels(1), + ); err != nil { return err } @@ -271,37 +272,6 @@ func (c *CodeServer) installSettings() error { return nil } -// downloadAndExtract downloads the release tarball at url to a temporary file -// (with retry and completeness verification) then extracts it under location. -// Cleans up a partial extraction on failure so retries start fresh. -func downloadAndExtract(url, location string) error { - tmpFile, err := os.CreateTemp("", "code-server-*.tar.gz") - if err != nil { - return fmt.Errorf("create temp file: %w", err) - } - tmpPath := tmpFile.Name() - _ = tmpFile.Close() - defer func() { _ = os.Remove(tmpPath) }() - - if err := devsyhttp.DownloadToFile(context.Background(), url, tmpPath); err != nil { - return err - } - - file, err := os.Open(filepath.Clean(tmpPath)) - if err != nil { - return err - } - defer func() { _ = file.Close() }() - - if err := extract.Extract(file, location, extract.StripLevels(1)); err != nil { - if rmErr := os.RemoveAll(location); rmErr != nil { - log.Warnf("cleanup partial install: path=%s err=%v", location, rmErr) - } - return fmt.Errorf("extract code-server: %w", err) - } - return nil -} - // suOrSh builds an *exec.Cmd that runs runCommand either as the unprivileged // user (via `su -c`) or as the current user (via `sh -c`). All arg // elements are constants from this package — runCommand itself is built from diff --git a/pkg/ide/download.go b/pkg/ide/download.go new file mode 100644 index 000000000..680df7dd1 --- /dev/null +++ b/pkg/ide/download.go @@ -0,0 +1,51 @@ +package ide + +import ( + "context" + "fmt" + "os" + "path/filepath" + + "github.com/devsy-org/devsy/pkg/extract" + devsyhttp "github.com/devsy-org/devsy/pkg/http" + "github.com/devsy-org/devsy/pkg/log" +) + +// DownloadAndExtract downloads the archive at url to a temporary file, then +// extracts it into destDir. The download (with retry and transfer-completeness +// verification) is delegated to devsyhttp.DownloadToFile, so a truncated +// transfer surfaces as a clear download error rather than an opaque +// decompression EOF; an extract failure therefore means a genuinely corrupt +// archive. On extract failure the partially-populated destDir is removed so a +// retry starts clean. Extract options (e.g. StripLevels) are passed through. +func DownloadAndExtract( + ctx context.Context, + url, destDir string, + opts ...extract.Option, +) error { + tmpFile, err := os.CreateTemp("", "devsy-ide-*.tar.gz") + if err != nil { + return fmt.Errorf("create temp file: %w", err) + } + tmpPath := tmpFile.Name() + _ = tmpFile.Close() + defer func() { _ = os.Remove(tmpPath) }() + + if err := devsyhttp.DownloadToFile(ctx, url, tmpPath); err != nil { + return err + } + + file, err := os.Open(filepath.Clean(tmpPath)) + if err != nil { + return err + } + defer func() { _ = file.Close() }() + + if err := extract.Extract(file, destDir, opts...); err != nil { + if rmErr := os.RemoveAll(destDir); rmErr != nil { + log.Warnf("cleanup partial install: path=%s err=%v", destDir, rmErr) + } + return fmt.Errorf("extract %s: %w", url, err) + } + return nil +} diff --git a/pkg/ide/download_test.go b/pkg/ide/download_test.go new file mode 100644 index 000000000..a83f6f556 --- /dev/null +++ b/pkg/ide/download_test.go @@ -0,0 +1,82 @@ +package ide + +import ( + "archive/tar" + "bytes" + "compress/gzip" + "context" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "strings" + "testing" +) + +// buildTarGz returns a gzipped tar holding a single file at its root. +func buildTarGz(t *testing.T, name, body string) []byte { + t.Helper() + var buf bytes.Buffer + gz := gzip.NewWriter(&buf) + tw := tar.NewWriter(gz) + if err := tw.WriteHeader( + &tar.Header{Name: name, Mode: 0o755, Size: int64(len(body))}, + ); err != nil { + t.Fatal(err) + } + if _, err := tw.Write([]byte(body)); err != nil { + t.Fatal(err) + } + if err := tw.Close(); err != nil { + t.Fatal(err) + } + if err := gz.Close(); err != nil { + t.Fatal(err) + } + return buf.Bytes() +} + +func TestDownloadAndExtractSuccess(t *testing.T) { + archive := buildTarGz(t, "code", "#!/bin/sh\necho code\n") + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write(archive) + })) + defer srv.Close() + + dest := t.TempDir() + if err := DownloadAndExtract(context.Background(), srv.URL, dest); err != nil { + t.Fatalf("DownloadAndExtract: %v", err) + } + if _, err := os.Stat(filepath.Join(dest, "code")); err != nil { + t.Fatalf("expected extracted file: %v", err) + } +} + +func TestDownloadAndExtractCorruptArchiveReportsExtractError(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + // A complete transfer of bytes that are not a valid archive: the + // download succeeds, so the failure must be attributed to extraction. + _, _ = w.Write([]byte("not-a-tarball")) + })) + defer srv.Close() + + err := DownloadAndExtract(context.Background(), srv.URL, t.TempDir()) + if err == nil || !strings.Contains(err.Error(), "extract") { + t.Fatalf("expected an extract-attributed error, got %v", err) + } +} + +func TestDownloadAndExtractCleansUpOnExtractFailure(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte("not-a-tarball")) + })) + defer srv.Close() + + dest := filepath.Join(t.TempDir(), "install") + if err := DownloadAndExtract(context.Background(), srv.URL, dest); err == nil { + t.Fatal("expected extract failure") + } + if _, err := os.Stat(dest); !os.IsNotExist(err) { + t.Fatalf("expected dest removed after extract failure, stat err = %v", err) + } +} diff --git a/pkg/ide/openvscode/openvscode.go b/pkg/ide/openvscode/openvscode.go index 965180f8f..c321810a2 100644 --- a/pkg/ide/openvscode/openvscode.go +++ b/pkg/ide/openvscode/openvscode.go @@ -13,7 +13,6 @@ import ( "github.com/devsy-org/devsy/pkg/config" copy2 "github.com/devsy-org/devsy/pkg/copy" "github.com/devsy-org/devsy/pkg/extract" - devsyhttp "github.com/devsy-org/devsy/pkg/http" "github.com/devsy-org/devsy/pkg/ide" "github.com/devsy-org/devsy/pkg/ide/vscode" "github.com/devsy-org/devsy/pkg/log" @@ -119,7 +118,9 @@ func (o *OpenVSCodeServer) Install() error { vscode.InstallAPKRequirements() // download tar - if err := downloadAndExtract(url, location); err != nil { + if err := ide.DownloadAndExtract( + context.Background(), url, location, extract.StripLevels(1), + ); err != nil { return err } @@ -136,33 +137,6 @@ func (o *OpenVSCodeServer) Install() error { return nil } -// downloadAndExtract downloads the openvscode-server tarball to a temporary file -// (with retry and completeness verification) then extracts it under location. -func downloadAndExtract(url, location string) error { - tmpFile, err := os.CreateTemp("", "openvscode-*.tar.gz") - if err != nil { - return fmt.Errorf("create temp file: %w", err) - } - tmpPath := tmpFile.Name() - _ = tmpFile.Close() - defer func() { _ = os.Remove(tmpPath) }() - - if err := devsyhttp.DownloadToFile(context.Background(), url, tmpPath); err != nil { - return err - } - - file, err := os.Open(filepath.Clean(tmpPath)) - if err != nil { - return err - } - defer func() { _ = file.Close() }() - - if err := extract.Extract(file, location, extract.StripLevels(1)); err != nil { - return fmt.Errorf("extract vscode: %w", err) - } - return nil -} - func chownIfNeeded(userName, location string) error { if userName == "" { return nil diff --git a/pkg/ide/vscodeweb/vscodeweb.go b/pkg/ide/vscodeweb/vscodeweb.go index 9ca2b9f0a..e3bde6fbd 100644 --- a/pkg/ide/vscodeweb/vscodeweb.go +++ b/pkg/ide/vscodeweb/vscodeweb.go @@ -12,8 +12,6 @@ import ( "github.com/devsy-org/devsy/pkg/command" "github.com/devsy-org/devsy/pkg/config" copy2 "github.com/devsy-org/devsy/pkg/copy" - "github.com/devsy-org/devsy/pkg/extract" - devsyhttp "github.com/devsy-org/devsy/pkg/http" "github.com/devsy-org/devsy/pkg/ide" "github.com/devsy-org/devsy/pkg/ide/vscode" "github.com/devsy-org/devsy/pkg/log" @@ -126,7 +124,7 @@ func (v *VSCodeWeb) Install() error { if !v.isInstalled(location, releaseURL) { vscode.InstallAPKRequirements() - if err := downloadAndExtract(releaseURL, location); err != nil { + if err := ide.DownloadAndExtract(context.Background(), releaseURL, location); err != nil { return err } @@ -296,41 +294,6 @@ func (v *VSCodeWeb) installSettings() error { return nil } -// downloadAndExtract downloads the CLI tarball at url to a temporary file, then -// extracts it under location. The download (with retry and transfer-completeness -// verification) is delegated to devsyhttp.DownloadToFile, so a truncated -// transfer is retried and reported as a clear download error; an extract failure -// here therefore means a genuinely corrupt archive. The VS Code CLI tarball -// holds a single `code` binary at its root, so no strip levels are applied. -// Cleans up a partial extraction on failure so retries start fresh. -func downloadAndExtract(url, location string) error { - tmpFile, err := os.CreateTemp("", "vscode-cli-*.tar.gz") - if err != nil { - return fmt.Errorf("create temp file for VS Code CLI: %w", err) - } - tmpPath := tmpFile.Name() - _ = tmpFile.Close() - defer func() { _ = os.Remove(tmpPath) }() - - if err := devsyhttp.DownloadToFile(context.Background(), url, tmpPath); err != nil { - return err - } - - file, err := os.Open(filepath.Clean(tmpPath)) - if err != nil { - return err - } - defer func() { _ = file.Close() }() - - if err := extract.Extract(file, location); err != nil { - if rmErr := os.RemoveAll(location); rmErr != nil { - log.Warnf("cleanup partial install: path=%s err=%v", location, rmErr) - } - return fmt.Errorf("extract VS Code CLI (archive may be corrupt): %w", err) - } - return nil -} - // suOrSh builds an *exec.Cmd that runs runCommand either as the unprivileged // user (via `su -c`) or as the current user (via `sh -c`). All arg // elements are constants from this package — runCommand itself is built from diff --git a/pkg/ide/vscodeweb/vscodeweb_test.go b/pkg/ide/vscodeweb/vscodeweb_test.go index 86f6fc9e1..927e938f7 100644 --- a/pkg/ide/vscodeweb/vscodeweb_test.go +++ b/pkg/ide/vscodeweb/vscodeweb_test.go @@ -1,11 +1,6 @@ package vscodeweb import ( - "archive/tar" - "bytes" - "compress/gzip" - "net/http" - "net/http/httptest" "os" "runtime" "strings" @@ -14,61 +9,6 @@ import ( "github.com/devsy-org/devsy/pkg/config" ) -// buildCodeTarGz returns a gzipped tar holding a single `code` binary at its -// root, matching the layout of the real VS Code CLI tarball. -func buildCodeTarGz(t *testing.T) []byte { - t.Helper() - var buf bytes.Buffer - gz := gzip.NewWriter(&buf) - tw := tar.NewWriter(gz) - content := []byte("#!/bin/sh\necho code\n") - if err := tw.WriteHeader( - &tar.Header{Name: "code", Mode: 0o755, Size: int64(len(content))}, - ); err != nil { - t.Fatal(err) - } - if _, err := tw.Write(content); err != nil { - t.Fatal(err) - } - if err := tw.Close(); err != nil { - t.Fatal(err) - } - if err := gz.Close(); err != nil { - t.Fatal(err) - } - return buf.Bytes() -} - -func TestDownloadAndExtractSuccess(t *testing.T) { - archive := buildCodeTarGz(t) - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { - _, _ = w.Write(archive) - })) - defer srv.Close() - - location := t.TempDir() - if err := downloadAndExtract(srv.URL, location); err != nil { - t.Fatalf("downloadAndExtract: %v", err) - } - if _, err := os.Stat(binaryPath(location)); err != nil { - t.Fatalf("expected extracted code binary: %v", err) - } -} - -func TestDownloadAndExtractCorruptArchiveReportsExtractError(t *testing.T) { - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { - // A complete transfer of bytes that are not a valid archive: the - // download succeeds, so the failure must be attributed to extraction. - _, _ = w.Write([]byte("not-a-tarball")) - })) - defer srv.Close() - - err := downloadAndExtract(srv.URL, t.TempDir()) - if err == nil || !strings.Contains(err.Error(), "extract VS Code CLI") { - t.Fatalf("expected an extract-attributed error, got %v", err) - } -} - func TestGetReleaseURLDefaultVersion(t *testing.T) { v := NewVSCodeWeb(ServerOptions{}) url := v.getReleaseURL() From 0e7d1475af09d59fcf31d6176f4ea3d2b088cec0 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Thu, 23 Jul 2026 10:35:04 -0500 Subject: [PATCH 06/16] fix: touchup comments Signed-off-by: Samuel K --- pkg/config/env.go | 4 +--- pkg/http/download.go | 9 +-------- pkg/http/http.go | 3 +-- pkg/ide/download.go | 7 +------ pkg/ide/download_test.go | 1 - pkg/ide/openvscode/openvscode.go | 3 --- pkg/ide/rstudio/rstudio.go | 2 +- 7 files changed, 5 insertions(+), 24 deletions(-) diff --git a/pkg/config/env.go b/pkg/config/env.go index dd3185cc1..bce1752ac 100644 --- a/pkg/config/env.go +++ b/pkg/config/env.go @@ -26,9 +26,7 @@ const ( EnvDebug = "DEVSY_DEBUG" // EnvInsecureTLS, when truthy, disables TLS certificate verification for the - // shared HTTP client. It exists only as an escape hatch for environments - // that must reach hosts with self-signed certificates; verification is on by - // default. + // shared HTTP client. Enabled by default. EnvInsecureTLS = "DEVSY_INSECURE_TLS" // EnvDisableTelemetry disables telemetry collection. diff --git a/pkg/http/download.go b/pkg/http/download.go index a02962d36..bb3602a8c 100644 --- a/pkg/http/download.go +++ b/pkg/http/download.go @@ -14,15 +14,12 @@ import ( "k8s.io/apimachinery/pkg/util/wait" ) -// DefaultDownloadBackoff retries a download three times with exponential -// backoff, matching the OCI pull retry policy used elsewhere. var DefaultDownloadBackoff = wait.Backoff{ Duration: 1 * time.Second, Factor: 2.0, Steps: 3, } -// downloadConfig holds the resolved options for a download. type downloadConfig struct { headers map[string]string backoff wait.Backoff @@ -78,11 +75,7 @@ func (e *permanentError) Unwrap() error { return e.err } // DownloadToFile downloads url into destPath, retrying transient failures // (connection errors, 5xx responses, and truncated transfers) with backoff and -// failing fast on permanent ones (4xx). Each attempt truncates destPath so a -// partial download never leaks into the next attempt, and the transfer is -// verified against Content-Length when the server advertises it — so an -// incomplete download surfaces as a clear error here rather than as a later -// decode failure in the caller. +// failing fast on permanent ones (4xx). func DownloadToFile(ctx context.Context, url, destPath string, opts ...DownloadOption) error { cfg := downloadConfig{backoff: DefaultDownloadBackoff, client: GetHTTPClient(), mode: 0o600} for _, opt := range opts { diff --git a/pkg/http/http.go b/pkg/http/http.go index 814384b5c..14188ad8e 100644 --- a/pkg/http/http.go +++ b/pkg/http/http.go @@ -20,8 +20,7 @@ func GetHTTPClient() *http.Client { httpClientOnce.Do(func() { customTransport := http.DefaultTransport.(*http.Transport).Clone() if insecureTLSEnabled() { - // #nosec G402 -- opt-in escape hatch (DEVSY_INSECURE_TLS) for - // environments that must reach hosts with self-signed certificates. + // #nosec G402 -- enabled with DEVSY_INSECURE_TLS env customTransport.TLSClientConfig = &tls.Config{InsecureSkipVerify: true} log.Warnf("TLS certificate verification is disabled (%s)", config.EnvInsecureTLS) } diff --git a/pkg/ide/download.go b/pkg/ide/download.go index 680df7dd1..26cc97869 100644 --- a/pkg/ide/download.go +++ b/pkg/ide/download.go @@ -12,12 +12,7 @@ import ( ) // DownloadAndExtract downloads the archive at url to a temporary file, then -// extracts it into destDir. The download (with retry and transfer-completeness -// verification) is delegated to devsyhttp.DownloadToFile, so a truncated -// transfer surfaces as a clear download error rather than an opaque -// decompression EOF; an extract failure therefore means a genuinely corrupt -// archive. On extract failure the partially-populated destDir is removed so a -// retry starts clean. Extract options (e.g. StripLevels) are passed through. +// extracts it into destDir. func DownloadAndExtract( ctx context.Context, url, destDir string, diff --git a/pkg/ide/download_test.go b/pkg/ide/download_test.go index a83f6f556..f405a462d 100644 --- a/pkg/ide/download_test.go +++ b/pkg/ide/download_test.go @@ -13,7 +13,6 @@ import ( "testing" ) -// buildTarGz returns a gzipped tar holding a single file at its root. func buildTarGz(t *testing.T, name, body string) []byte { t.Helper() var buf bytes.Buffer diff --git a/pkg/ide/openvscode/openvscode.go b/pkg/ide/openvscode/openvscode.go index c321810a2..d087be096 100644 --- a/pkg/ide/openvscode/openvscode.go +++ b/pkg/ide/openvscode/openvscode.go @@ -117,19 +117,16 @@ func (o *OpenVSCodeServer) Install() error { vscode.InstallAPKRequirements() - // download tar if err := ide.DownloadAndExtract( context.Background(), url, location, extract.StripLevels(1), ); err != nil { return err } - // chown location if err := chownIfNeeded(o.userName, location); err != nil { return err } - // paste settings if err := o.installSettings(); err != nil { return fmt.Errorf("install settings: %w", err) } diff --git a/pkg/ide/rstudio/rstudio.go b/pkg/ide/rstudio/rstudio.go index 595292ace..4e4dee80c 100644 --- a/pkg/ide/rstudio/rstudio.go +++ b/pkg/ide/rstudio/rstudio.go @@ -215,7 +215,7 @@ func getDownloadURL(version, ubuntuCodename, architecture string) string { } func download(targetFolder, downloadURL string) (string, error) { - // #nosec G301 -- IDE install dir; matches other IDE download destinations. + // #nosec G301 -- IDE install dir if err := os.MkdirAll(targetFolder, 0o755); err != nil { return "", err } From b8956958dfd8a3391fe7cbc664b0189f9be0f173 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Thu, 23 Jul 2026 15:14:59 -0500 Subject: [PATCH 07/16] fix(download): write downloads atomically and stage extractions - copyToFile writes to a temp file and renames on success, so a failed transfer never leaves a partial file at the destination (fixes callers like fleet that detect installs via os.Stat existence). - DownloadAndExtract extracts into a staging dir and swaps it into place only on success, preserving a pre-existing install if extraction fails. --- pkg/http/download.go | 36 +++++++++++++++++++++---- pkg/http/download_test.go | 48 ++++++++++++++++++++++++++++++++++ pkg/ide/download.go | 55 ++++++++++++++++++++++++++++++++++++--- pkg/ide/download_test.go | 55 +++++++++++++++++++++++++++++++++++++++ 4 files changed, 186 insertions(+), 8 deletions(-) diff --git a/pkg/http/download.go b/pkg/http/download.go index bb3602a8c..3dec5f57d 100644 --- a/pkg/http/download.go +++ b/pkg/http/download.go @@ -141,26 +141,52 @@ func copyToFile(resp *http.Response, cfg *downloadConfig, destPath, url string) return nil } + destPath = filepath.Clean(destPath) + destDir := filepath.Dir(destPath) // #nosec G301 -- match existing download destinations; contents are public artifacts. - if err := os.MkdirAll(filepath.Dir(destPath), 0o755); err != nil { + if err := os.MkdirAll(destDir, 0o755); err != nil { return &permanentError{fmt.Errorf("create download folder: %w", err)} } - file, err := os.OpenFile(filepath.Clean(destPath), os.O_CREATE|os.O_WRONLY|os.O_TRUNC, cfg.mode) + // Download to a temporary file in the destination directory and rename it + // into place only on success, so a failed transfer never leaves a partial + // file at destPath (which callers may mistake for a complete download). + tmp, err := os.CreateTemp(destDir, "."+filepath.Base(destPath)+".*.tmp") if err != nil { return &permanentError{fmt.Errorf("create download file: %w", err)} } - defer func() { _ = file.Close() }() + tmpPath := tmp.Name() + committed := false + defer func() { + _ = tmp.Close() + if !committed { + _ = os.Remove(tmpPath) + } + }() + + if err := tmp.Chmod(cfg.mode); err != nil { + return &permanentError{fmt.Errorf("set download file mode: %w", err)} + } src := io.Reader(resp.Body) if cfg.wrap != nil { src = cfg.wrap(resp.Body, resp.ContentLength) } - written, err := io.Copy(file, src) + written, err := io.Copy(tmp, src) if err != nil { return fmt.Errorf("download %s: %w", url, err) } - return verifyContentLength(resp, written, url) + if err := verifyContentLength(resp, written, url); err != nil { + return err + } + if err := tmp.Close(); err != nil { + return fmt.Errorf("download %s: %w", url, err) + } + if err := os.Rename(tmpPath, destPath); err != nil { + return fmt.Errorf("download %s: %w", url, err) + } + committed = true + return nil } // alreadyDownloaded reports whether destPath already holds the full download, diff --git a/pkg/http/download_test.go b/pkg/http/download_test.go index aaae8eabb..fd0112a84 100644 --- a/pkg/http/download_test.go +++ b/pkg/http/download_test.go @@ -80,6 +80,54 @@ func TestDownloadToFileTruncatedRetriesThenReportsClearError(t *testing.T) { } } +func TestDownloadToFileLeavesNoPartialFileOnFailure(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Length", "1000") + _, _ = w.Write([]byte("short")) + })) + defer srv.Close() + + dest := filepath.Join(t.TempDir(), "out.bin") + if err := DownloadToFile(context.Background(), srv.URL, dest, fastBackoff()); err == nil { + t.Fatal("expected error for repeated truncated transfers") + } + if _, err := os.Stat(dest); !os.IsNotExist(err) { + t.Fatalf("expected no partial file at destination, stat err = %v", err) + } + entries, err := os.ReadDir(filepath.Dir(dest)) + if err != nil { + t.Fatal(err) + } + if len(entries) != 0 { + t.Fatalf("expected no leftover temp files, found %d entries", len(entries)) + } +} + +func TestDownloadToFilePreservesExistingFileOnFailure(t *testing.T) { + const existing = "previous-good-content" + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Length", "1000") + _, _ = w.Write([]byte("short")) + })) + defer srv.Close() + + dest := filepath.Join(t.TempDir(), "out.bin") + if err := os.WriteFile(dest, []byte(existing), 0o600); err != nil { + t.Fatal(err) + } + + if err := DownloadToFile(context.Background(), srv.URL, dest, fastBackoff()); err == nil { + t.Fatal("expected error for repeated truncated transfers") + } + got, err := os.ReadFile(dest) // #nosec G304 -- test-controlled temp path + if err != nil { + t.Fatalf("expected existing file preserved, got %v", err) + } + if string(got) != existing { + t.Fatalf("existing file was clobbered, got %q", got) + } +} + func TestDownloadToFileRecoversAfterTransientFailure(t *testing.T) { const body = "recovered-payload" var calls int diff --git a/pkg/ide/download.go b/pkg/ide/download.go index 26cc97869..e93e5881a 100644 --- a/pkg/ide/download.go +++ b/pkg/ide/download.go @@ -36,11 +36,60 @@ func DownloadAndExtract( } defer func() { _ = file.Close() }() - if err := extract.Extract(file, destDir, opts...); err != nil { - if rmErr := os.RemoveAll(destDir); rmErr != nil { - log.Warnf("cleanup partial install: path=%s err=%v", destDir, rmErr) + // Extract into a sibling staging directory and only swap it into destDir on + // success, so a failed extraction never destroys a pre-existing install. + if err := os.MkdirAll(filepath.Dir(destDir), 0o755); err != nil { + return fmt.Errorf("create install parent: %w", err) + } + stagingDir, err := os.MkdirTemp(filepath.Dir(destDir), "."+filepath.Base(destDir)+".staging.*") + if err != nil { + return fmt.Errorf("create staging dir: %w", err) + } + swapped := false + defer func() { + if !swapped { + if rmErr := os.RemoveAll(stagingDir); rmErr != nil { + log.Warnf("cleanup staging dir: path=%s err=%v", stagingDir, rmErr) + } } + }() + + if err := extract.Extract(file, stagingDir, opts...); err != nil { return fmt.Errorf("extract %s: %w", url, err) } + + return swapIntoPlace(stagingDir, destDir, &swapped) +} + +// swapIntoPlace replaces destDir with stagingDir, preserving the previous +// contents until the swap succeeds so a rename failure cannot leave destDir +// missing. +func swapIntoPlace(stagingDir, destDir string, swapped *bool) error { + backupDir := stagingDir + ".old" + hasBackup := false + if _, err := os.Stat(destDir); err == nil { + if err := os.Rename(destDir, backupDir); err != nil { + return fmt.Errorf("stage existing install: %w", err) + } + hasBackup = true + } else if !os.IsNotExist(err) { + return fmt.Errorf("inspect install dir: %w", err) + } + + if err := os.Rename(stagingDir, destDir); err != nil { + if hasBackup { + if restoreErr := os.Rename(backupDir, destDir); restoreErr != nil { + log.Warnf("restore install dir: path=%s err=%v", destDir, restoreErr) + } + } + return fmt.Errorf("install to %s: %w", destDir, err) + } + *swapped = true + + if hasBackup { + if rmErr := os.RemoveAll(backupDir); rmErr != nil { + log.Warnf("cleanup old install: path=%s err=%v", backupDir, rmErr) + } + } return nil } diff --git a/pkg/ide/download_test.go b/pkg/ide/download_test.go index f405a462d..417ea7612 100644 --- a/pkg/ide/download_test.go +++ b/pkg/ide/download_test.go @@ -79,3 +79,58 @@ func TestDownloadAndExtractCleansUpOnExtractFailure(t *testing.T) { t.Fatalf("expected dest removed after extract failure, stat err = %v", err) } } + +func TestDownloadAndExtractPreservesExistingInstallOnFailure(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte("not-a-tarball")) + })) + defer srv.Close() + + dest := filepath.Join(t.TempDir(), "install") + if err := os.MkdirAll(dest, 0o755); err != nil { + t.Fatal(err) + } + existing := filepath.Join(dest, "existing") + if err := os.WriteFile(existing, []byte("keep-me"), 0o644); err != nil { + t.Fatal(err) + } + + if err := DownloadAndExtract(context.Background(), srv.URL, dest); err == nil { + t.Fatal("expected extract failure") + } + + got, err := os.ReadFile(existing) + if err != nil { + t.Fatalf("expected pre-existing install to remain intact: %v", err) + } + if string(got) != "keep-me" { + t.Fatalf("pre-existing content changed, got %q", got) + } +} + +func TestDownloadAndExtractReplacesExistingInstallOnSuccess(t *testing.T) { + archive := buildTarGz(t, "code", "#!/bin/sh\necho code\n") + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write(archive) + })) + defer srv.Close() + + dest := filepath.Join(t.TempDir(), "install") + if err := os.MkdirAll(dest, 0o755); err != nil { + t.Fatal(err) + } + stale := filepath.Join(dest, "stale") + if err := os.WriteFile(stale, []byte("old"), 0o644); err != nil { + t.Fatal(err) + } + + if err := DownloadAndExtract(context.Background(), srv.URL, dest); err != nil { + t.Fatalf("DownloadAndExtract: %v", err) + } + if _, err := os.Stat(filepath.Join(dest, "code")); err != nil { + t.Fatalf("expected extracted file: %v", err) + } + if _, err := os.Stat(stale); !os.IsNotExist(err) { + t.Fatalf("expected stale file replaced, stat err = %v", err) + } +} From d438c88cfacaaa2715f7839082ac24f1a8c880c9 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Thu, 23 Jul 2026 15:22:59 -0500 Subject: [PATCH 08/16] fix(download): abort stalled downloads via inactivity watchdog The shared HTTP client sets no Client.Timeout and download callers pass context.Background(), so an unresponsive endpoint could block a transfer indefinitely. Add a per-attempt stall timeout (default 60s) that cancels an attempt when no response headers arrive or no bytes are read within the window, while a steadily-progressing transfer keeps rescheduling the deadline. Stalls surface as retryable errors. --- pkg/http/download.go | 126 +++++++++++++++++++++++++++++++++++--- pkg/http/download_test.go | 35 +++++++++++ 2 files changed, 152 insertions(+), 9 deletions(-) diff --git a/pkg/http/download.go b/pkg/http/download.go index 3dec5f57d..e5133871f 100644 --- a/pkg/http/download.go +++ b/pkg/http/download.go @@ -8,6 +8,7 @@ import ( "net/http" "os" "path/filepath" + "sync" "time" "github.com/devsy-org/devsy/pkg/log" @@ -20,13 +21,21 @@ var DefaultDownloadBackoff = wait.Backoff{ Steps: 3, } +// DefaultStallTimeout bounds how long a download attempt may make no progress +// (no response headers or no bytes read) before it is aborted and retried. It +// guards against unresponsive endpoints without penalizing large downloads +// that are still transferring data, since the shared HTTP client has no +// overall Client.Timeout. +const DefaultStallTimeout = 60 * time.Second + type downloadConfig struct { - headers map[string]string - backoff wait.Backoff - client *http.Client - mode os.FileMode - wrap func(r io.Reader, totalSize int64) io.Reader - skipIfSized bool + headers map[string]string + backoff wait.Backoff + client *http.Client + mode os.FileMode + wrap func(r io.Reader, totalSize int64) io.Reader + skipIfSized bool + stallTimeout time.Duration } // DownloadOption customizes DownloadToFile. @@ -66,6 +75,12 @@ func SkipIfSameSize() DownloadOption { return func(c *downloadConfig) { c.skipIfSized = true } } +// WithStallTimeout overrides how long an attempt may make no progress before it +// is aborted. A non-positive value disables the stall guard entirely. +func WithStallTimeout(d time.Duration) DownloadOption { + return func(c *downloadConfig) { c.stallTimeout = d } +} + // permanentError marks a failure that will not be resolved by retrying, such as // a 4xx response or a malformed request. type permanentError struct{ err error } @@ -77,7 +92,12 @@ func (e *permanentError) Unwrap() error { return e.err } // (connection errors, 5xx responses, and truncated transfers) with backoff and // failing fast on permanent ones (4xx). func DownloadToFile(ctx context.Context, url, destPath string, opts ...DownloadOption) error { - cfg := downloadConfig{backoff: DefaultDownloadBackoff, client: GetHTTPClient(), mode: 0o600} + cfg := downloadConfig{ + backoff: DefaultDownloadBackoff, + client: GetHTTPClient(), + mode: 0o600, + stallTimeout: DefaultStallTimeout, + } for _, opt := range opts { opt(&cfg) } @@ -111,6 +131,27 @@ func isPermanent(err error) bool { } func downloadOnce(ctx context.Context, cfg *downloadConfig, url, destPath string) error { + var wd *stallWatchdog + if cfg.stallTimeout > 0 { + var cancel context.CancelFunc + ctx, cancel = context.WithCancel(ctx) + defer cancel() + wd = newStallWatchdog(cfg.stallTimeout, cancel) + defer wd.stop() + } + + err := doDownload(ctx, cfg, url, destPath, wd) + // A stall aborts via context cancellation; surface it as a clear, retryable + // error instead of a bare "context canceled". + if wd != nil && wd.fired() && errors.Is(err, context.Canceled) { + return fmt.Errorf("download %s: stalled for %s", url, cfg.stallTimeout) + } + return err +} + +func doDownload( + ctx context.Context, cfg *downloadConfig, url, destPath string, wd *stallWatchdog, +) error { req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil) if err != nil { return &permanentError{fmt.Errorf("download %s: %w", url, err)} @@ -125,6 +166,11 @@ func downloadOnce(ctx context.Context, cfg *downloadConfig, url, destPath string } defer func() { _ = resp.Body.Close() }() + // Headers received: give the body transfer a fresh inactivity window. + if wd != nil { + wd.reset() + } + if resp.StatusCode >= http.StatusBadRequest { statusErr := fmt.Errorf("download %s: %s", url, resp.Status) if resp.StatusCode < http.StatusInternalServerError { @@ -133,10 +179,12 @@ func downloadOnce(ctx context.Context, cfg *downloadConfig, url, destPath string return statusErr } - return copyToFile(resp, cfg, destPath, url) + return copyToFile(resp, cfg, destPath, url, wd) } -func copyToFile(resp *http.Response, cfg *downloadConfig, destPath, url string) error { +func copyToFile( + resp *http.Response, cfg *downloadConfig, destPath, url string, wd *stallWatchdog, +) error { if alreadyDownloaded(cfg, resp, destPath) { return nil } @@ -172,6 +220,9 @@ func copyToFile(resp *http.Response, cfg *downloadConfig, destPath, url string) if cfg.wrap != nil { src = cfg.wrap(resp.Body, resp.ContentLength) } + if wd != nil { + src = &stallResetReader{r: src, wd: wd} + } written, err := io.Copy(tmp, src) if err != nil { return fmt.Errorf("download %s: %w", url, err) @@ -208,3 +259,60 @@ func verifyContentLength(resp *http.Response, written int64, url string) error { } return nil } + +// stallWatchdog aborts a download attempt (by cancelling its context) when no +// progress is made within timeout. Each observed unit of progress reschedules +// the deadline. +type stallWatchdog struct { + timeout time.Duration + timer *time.Timer + mu sync.Mutex + triggered bool +} + +func newStallWatchdog(timeout time.Duration, cancel context.CancelFunc) *stallWatchdog { + w := &stallWatchdog{timeout: timeout} + w.timer = time.AfterFunc(timeout, func() { + w.mu.Lock() + w.triggered = true + w.mu.Unlock() + cancel() + }) + return w +} + +func (w *stallWatchdog) reset() { + w.mu.Lock() + defer w.mu.Unlock() + if w.triggered { + return + } + w.timer.Reset(w.timeout) +} + +func (w *stallWatchdog) stop() { + w.mu.Lock() + defer w.mu.Unlock() + w.timer.Stop() +} + +func (w *stallWatchdog) fired() bool { + w.mu.Lock() + defer w.mu.Unlock() + return w.triggered +} + +// stallResetReader reschedules the watchdog deadline on each successful read so +// a steadily-progressing transfer is never aborted. +type stallResetReader struct { + r io.Reader + wd *stallWatchdog +} + +func (s *stallResetReader) Read(p []byte) (int, error) { + n, err := s.r.Read(p) + if n > 0 { + s.wd.reset() + } + return n, err +} diff --git a/pkg/http/download_test.go b/pkg/http/download_test.go index fd0112a84..c0c44c570 100644 --- a/pkg/http/download_test.go +++ b/pkg/http/download_test.go @@ -128,6 +128,41 @@ func TestDownloadToFilePreservesExistingFileOnFailure(t *testing.T) { } } +func TestDownloadToFileStallTimeoutAborts(t *testing.T) { + release := make(chan struct{}) + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Length", "1000") + w.WriteHeader(http.StatusOK) + flusher, ok := w.(http.Flusher) + if !ok { + t.Error("expected flusher") + return + } + _, _ = w.Write([]byte("partial")) + flusher.Flush() + // Send headers and a little data, then hang: the stall watchdog must + // abort the attempt rather than block forever. + select { + case <-r.Context().Done(): + case <-release: + } + })) + defer srv.Close() + defer close(release) + + dest := filepath.Join(t.TempDir(), "out.bin") + err := DownloadToFile( + context.Background(), srv.URL, dest, + WithStallTimeout(50*time.Millisecond), fastBackoff(), + ) + if err == nil || !strings.Contains(err.Error(), "stalled") { + t.Fatalf("expected a stalled error, got %v", err) + } + if _, statErr := os.Stat(dest); !os.IsNotExist(statErr) { + t.Fatalf("expected no partial file after stall, stat err = %v", statErr) + } +} + func TestDownloadToFileRecoversAfterTransientFailure(t *testing.T) { const body = "recovered-payload" var calls int From fb672bcdae96e36c5e29db668aa4611b30a6f467 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Thu, 23 Jul 2026 15:29:13 -0500 Subject: [PATCH 09/16] refactor(download): satisfy cyclop/revive/gosec lint - Split copyToFile and DownloadAndExtract into smaller helpers to stay under the cyclomatic-complexity limit. - Group per-attempt download state into a struct so helpers stay within the 4-argument limit. - Use 0o750 dir / 0o600 file perms in tests and annotate the IDE install dir MkdirAll and test ReadFile for gosec. --- pkg/http/download.go | 106 ++++++++++++++++++++++++--------------- pkg/ide/download.go | 92 ++++++++++++++++++++++----------- pkg/ide/download_test.go | 10 ++-- 3 files changed, 132 insertions(+), 76 deletions(-) diff --git a/pkg/http/download.go b/pkg/http/download.go index e5133871f..49d9f32c9 100644 --- a/pkg/http/download.go +++ b/pkg/http/download.go @@ -130,66 +130,71 @@ func isPermanent(err error) bool { return errors.As(err, &perr) } +// downloadAttempt carries the per-attempt state so helpers stay within the +// project's argument limit. +type downloadAttempt struct { + cfg *downloadConfig + url string + destPath string + wd *stallWatchdog +} + func downloadOnce(ctx context.Context, cfg *downloadConfig, url, destPath string) error { - var wd *stallWatchdog + a := &downloadAttempt{cfg: cfg, url: url, destPath: destPath} if cfg.stallTimeout > 0 { var cancel context.CancelFunc ctx, cancel = context.WithCancel(ctx) defer cancel() - wd = newStallWatchdog(cfg.stallTimeout, cancel) - defer wd.stop() + a.wd = newStallWatchdog(cfg.stallTimeout, cancel) + defer a.wd.stop() } - err := doDownload(ctx, cfg, url, destPath, wd) + err := a.do(ctx) // A stall aborts via context cancellation; surface it as a clear, retryable // error instead of a bare "context canceled". - if wd != nil && wd.fired() && errors.Is(err, context.Canceled) { + if a.wd != nil && a.wd.fired() && errors.Is(err, context.Canceled) { return fmt.Errorf("download %s: stalled for %s", url, cfg.stallTimeout) } return err } -func doDownload( - ctx context.Context, cfg *downloadConfig, url, destPath string, wd *stallWatchdog, -) error { - req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil) +func (a *downloadAttempt) do(ctx context.Context) error { + req, err := http.NewRequestWithContext(ctx, http.MethodGet, a.url, nil) if err != nil { - return &permanentError{fmt.Errorf("download %s: %w", url, err)} + return &permanentError{fmt.Errorf("download %s: %w", a.url, err)} } - for key, value := range cfg.headers { + for key, value := range a.cfg.headers { req.Header.Set(key, value) } - resp, err := cfg.client.Do(req) + resp, err := a.cfg.client.Do(req) if err != nil { - return fmt.Errorf("download %s: %w", url, err) + return fmt.Errorf("download %s: %w", a.url, err) } defer func() { _ = resp.Body.Close() }() // Headers received: give the body transfer a fresh inactivity window. - if wd != nil { - wd.reset() + if a.wd != nil { + a.wd.reset() } if resp.StatusCode >= http.StatusBadRequest { - statusErr := fmt.Errorf("download %s: %s", url, resp.Status) + statusErr := fmt.Errorf("download %s: %s", a.url, resp.Status) if resp.StatusCode < http.StatusInternalServerError { return &permanentError{statusErr} } return statusErr } - return copyToFile(resp, cfg, destPath, url, wd) + return a.copyToFile(resp) } -func copyToFile( - resp *http.Response, cfg *downloadConfig, destPath, url string, wd *stallWatchdog, -) error { - if alreadyDownloaded(cfg, resp, destPath) { +func (a *downloadAttempt) copyToFile(resp *http.Response) error { + if alreadyDownloaded(a.cfg, resp, a.destPath) { return nil } - destPath = filepath.Clean(destPath) + destPath := filepath.Clean(a.destPath) destDir := filepath.Dir(destPath) // #nosec G301 -- match existing download destinations; contents are public artifacts. if err := os.MkdirAll(destDir, 0o755); err != nil { @@ -199,9 +204,25 @@ func copyToFile( // Download to a temporary file in the destination directory and rename it // into place only on success, so a failed transfer never leaves a partial // file at destPath (which callers may mistake for a complete download). + tmpPath, err := a.streamToTempFile(resp, destDir, destPath) + if err != nil { + return err + } + if err := os.Rename(tmpPath, destPath); err != nil { + _ = os.Remove(tmpPath) + return fmt.Errorf("download %s: %w", a.url, err) + } + return nil +} + +// streamToTempFile writes the response body to a temporary file next to +// destPath and returns its path on success, removing it on any failure. +func (a *downloadAttempt) streamToTempFile( + resp *http.Response, destDir, destPath string, +) (string, error) { tmp, err := os.CreateTemp(destDir, "."+filepath.Base(destPath)+".*.tmp") if err != nil { - return &permanentError{fmt.Errorf("create download file: %w", err)} + return "", &permanentError{fmt.Errorf("create download file: %w", err)} } tmpPath := tmp.Name() committed := false @@ -212,32 +233,35 @@ func copyToFile( } }() - if err := tmp.Chmod(cfg.mode); err != nil { - return &permanentError{fmt.Errorf("set download file mode: %w", err)} + if err := tmp.Chmod(a.cfg.mode); err != nil { + return "", &permanentError{fmt.Errorf("set download file mode: %w", err)} } - src := io.Reader(resp.Body) - if cfg.wrap != nil { - src = cfg.wrap(resp.Body, resp.ContentLength) - } - if wd != nil { - src = &stallResetReader{r: src, wd: wd} - } - written, err := io.Copy(tmp, src) + written, err := io.Copy(tmp, a.source(resp)) if err != nil { - return fmt.Errorf("download %s: %w", url, err) + return "", fmt.Errorf("download %s: %w", a.url, err) } - if err := verifyContentLength(resp, written, url); err != nil { - return err + if err := verifyContentLength(resp, written, a.url); err != nil { + return "", err } if err := tmp.Close(); err != nil { - return fmt.Errorf("download %s: %w", url, err) - } - if err := os.Rename(tmpPath, destPath); err != nil { - return fmt.Errorf("download %s: %w", url, err) + return "", fmt.Errorf("download %s: %w", a.url, err) } committed = true - return nil + return tmpPath, nil +} + +// source builds the reader copied to disk, applying the optional progress +// wrapper and stall watchdog. +func (a *downloadAttempt) source(resp *http.Response) io.Reader { + src := io.Reader(resp.Body) + if a.cfg.wrap != nil { + src = a.cfg.wrap(resp.Body, resp.ContentLength) + } + if a.wd != nil { + src = &stallResetReader{r: src, wd: a.wd} + } + return src } // alreadyDownloaded reports whether destPath already holds the full download, diff --git a/pkg/ide/download.go b/pkg/ide/download.go index e93e5881a..f3f53119c 100644 --- a/pkg/ide/download.go +++ b/pkg/ide/download.go @@ -12,71 +12,88 @@ import ( ) // DownloadAndExtract downloads the archive at url to a temporary file, then -// extracts it into destDir. +// extracts it into destDir. The archive is unpacked into a sibling staging +// directory and swapped into place only on success, so a failed download or +// extraction never destroys a pre-existing install. func DownloadAndExtract( ctx context.Context, url, destDir string, opts ...extract.Option, ) error { + tmpPath, err := downloadToTemp(ctx, url) + if err != nil { + return err + } + defer func() { _ = os.Remove(tmpPath) }() + + stagingDir, err := extractToStaging(url, tmpPath, destDir, opts...) + if err != nil { + return err + } + return swapIntoPlace(stagingDir, destDir) +} + +// downloadToTemp downloads url into a temporary file and returns its path. The +// temporary file is removed on failure. +func downloadToTemp(ctx context.Context, url string) (string, error) { tmpFile, err := os.CreateTemp("", "devsy-ide-*.tar.gz") if err != nil { - return fmt.Errorf("create temp file: %w", err) + return "", fmt.Errorf("create temp file: %w", err) } tmpPath := tmpFile.Name() _ = tmpFile.Close() - defer func() { _ = os.Remove(tmpPath) }() if err := devsyhttp.DownloadToFile(ctx, url, tmpPath); err != nil { - return err + _ = os.Remove(tmpPath) + return "", err } + return tmpPath, nil +} +// extractToStaging unpacks the archive at tmpPath into a sibling staging +// directory of destDir and returns its path. The staging directory is removed +// if extraction fails. +func extractToStaging( + url, tmpPath, destDir string, + opts ...extract.Option, +) (string, error) { file, err := os.Open(filepath.Clean(tmpPath)) if err != nil { - return err + return "", err } defer func() { _ = file.Close() }() - // Extract into a sibling staging directory and only swap it into destDir on - // success, so a failed extraction never destroys a pre-existing install. + // #nosec G301 -- IDE install dir if err := os.MkdirAll(filepath.Dir(destDir), 0o755); err != nil { - return fmt.Errorf("create install parent: %w", err) + return "", fmt.Errorf("create install parent: %w", err) } stagingDir, err := os.MkdirTemp(filepath.Dir(destDir), "."+filepath.Base(destDir)+".staging.*") if err != nil { - return fmt.Errorf("create staging dir: %w", err) + return "", fmt.Errorf("create staging dir: %w", err) } - swapped := false - defer func() { - if !swapped { - if rmErr := os.RemoveAll(stagingDir); rmErr != nil { - log.Warnf("cleanup staging dir: path=%s err=%v", stagingDir, rmErr) - } - } - }() if err := extract.Extract(file, stagingDir, opts...); err != nil { - return fmt.Errorf("extract %s: %w", url, err) + if rmErr := os.RemoveAll(stagingDir); rmErr != nil { + log.Warnf("cleanup staging dir: path=%s err=%v", stagingDir, rmErr) + } + return "", fmt.Errorf("extract %s: %w", url, err) } - - return swapIntoPlace(stagingDir, destDir, &swapped) + return stagingDir, nil } // swapIntoPlace replaces destDir with stagingDir, preserving the previous // contents until the swap succeeds so a rename failure cannot leave destDir // missing. -func swapIntoPlace(stagingDir, destDir string, swapped *bool) error { +func swapIntoPlace(stagingDir, destDir string) error { backupDir := stagingDir + ".old" - hasBackup := false - if _, err := os.Stat(destDir); err == nil { - if err := os.Rename(destDir, backupDir); err != nil { - return fmt.Errorf("stage existing install: %w", err) - } - hasBackup = true - } else if !os.IsNotExist(err) { - return fmt.Errorf("inspect install dir: %w", err) + hasBackup, err := stashExisting(destDir, backupDir) + if err != nil { + _ = os.RemoveAll(stagingDir) + return err } if err := os.Rename(stagingDir, destDir); err != nil { + _ = os.RemoveAll(stagingDir) if hasBackup { if restoreErr := os.Rename(backupDir, destDir); restoreErr != nil { log.Warnf("restore install dir: path=%s err=%v", destDir, restoreErr) @@ -84,7 +101,6 @@ func swapIntoPlace(stagingDir, destDir string, swapped *bool) error { } return fmt.Errorf("install to %s: %w", destDir, err) } - *swapped = true if hasBackup { if rmErr := os.RemoveAll(backupDir); rmErr != nil { @@ -93,3 +109,19 @@ func swapIntoPlace(stagingDir, destDir string, swapped *bool) error { } return nil } + +// stashExisting moves an existing destDir aside to backupDir so it can be +// restored if the subsequent swap fails. It reports whether a backup was made. +func stashExisting(destDir, backupDir string) (bool, error) { + _, err := os.Stat(destDir) + if os.IsNotExist(err) { + return false, nil + } + if err != nil { + return false, fmt.Errorf("inspect install dir: %w", err) + } + if err := os.Rename(destDir, backupDir); err != nil { + return false, fmt.Errorf("stage existing install: %w", err) + } + return true, nil +} diff --git a/pkg/ide/download_test.go b/pkg/ide/download_test.go index 417ea7612..ad20f1828 100644 --- a/pkg/ide/download_test.go +++ b/pkg/ide/download_test.go @@ -87,11 +87,11 @@ func TestDownloadAndExtractPreservesExistingInstallOnFailure(t *testing.T) { defer srv.Close() dest := filepath.Join(t.TempDir(), "install") - if err := os.MkdirAll(dest, 0o755); err != nil { + if err := os.MkdirAll(dest, 0o750); err != nil { t.Fatal(err) } existing := filepath.Join(dest, "existing") - if err := os.WriteFile(existing, []byte("keep-me"), 0o644); err != nil { + if err := os.WriteFile(existing, []byte("keep-me"), 0o600); err != nil { t.Fatal(err) } @@ -99,7 +99,7 @@ func TestDownloadAndExtractPreservesExistingInstallOnFailure(t *testing.T) { t.Fatal("expected extract failure") } - got, err := os.ReadFile(existing) + got, err := os.ReadFile(existing) // #nosec G304 -- test-controlled temp path if err != nil { t.Fatalf("expected pre-existing install to remain intact: %v", err) } @@ -116,11 +116,11 @@ func TestDownloadAndExtractReplacesExistingInstallOnSuccess(t *testing.T) { defer srv.Close() dest := filepath.Join(t.TempDir(), "install") - if err := os.MkdirAll(dest, 0o755); err != nil { + if err := os.MkdirAll(dest, 0o750); err != nil { t.Fatal(err) } stale := filepath.Join(dest, "stale") - if err := os.WriteFile(stale, []byte("old"), 0o644); err != nil { + if err := os.WriteFile(stale, []byte("old"), 0o600); err != nil { t.Fatal(err) } From 74ec75ef6837549bd96a3162f5cc56f79b48c58b Mon Sep 17 00:00:00 2001 From: Samuel K Date: Thu, 23 Jul 2026 16:36:23 -0500 Subject: [PATCH 10/16] fix(download): retry 429 and preserve install dir permissions - Treat 429 Too Many Requests as a retryable status instead of a permanent 4xx failure, matching its retry-later semantics. - Restore the 0755 install-dir mode: os.MkdirTemp creates the staging dir as 0700, so chmod it back to the mode extract.Extract previously produced for destDir before swapping it into place. --- pkg/http/download.go | 17 ++++++++++++----- pkg/http/download_test.go | 18 ++++++++++++++++++ pkg/ide/download.go | 18 +++++++++++++++--- pkg/ide/download_test.go | 7 +++++++ 4 files changed, 52 insertions(+), 8 deletions(-) diff --git a/pkg/http/download.go b/pkg/http/download.go index 49d9f32c9..d2a1ea554 100644 --- a/pkg/http/download.go +++ b/pkg/http/download.go @@ -89,8 +89,8 @@ func (e *permanentError) Error() string { return e.err.Error() } func (e *permanentError) Unwrap() error { return e.err } // DownloadToFile downloads url into destPath, retrying transient failures -// (connection errors, 5xx responses, and truncated transfers) with backoff and -// failing fast on permanent ones (4xx). +// (connection errors, 5xx responses, 429 Too Many Requests, and truncated +// transfers) with backoff and failing fast on permanent ones (other 4xx). func DownloadToFile(ctx context.Context, url, destPath string, opts ...DownloadOption) error { cfg := downloadConfig{ backoff: DefaultDownloadBackoff, @@ -180,15 +180,22 @@ func (a *downloadAttempt) do(ctx context.Context) error { if resp.StatusCode >= http.StatusBadRequest { statusErr := fmt.Errorf("download %s: %s", a.url, resp.Status) - if resp.StatusCode < http.StatusInternalServerError { - return &permanentError{statusErr} + if retryableStatus(resp.StatusCode) { + return statusErr } - return statusErr + return &permanentError{statusErr} } return a.copyToFile(resp) } +// retryableStatus reports whether an HTTP error status should be retried. 5xx +// responses are transient, and 429 Too Many Requests is an explicit +// retry-later signal; all other 4xx statuses are treated as permanent. +func retryableStatus(code int) bool { + return code >= http.StatusInternalServerError || code == http.StatusTooManyRequests +} + func (a *downloadAttempt) copyToFile(resp *http.Response) error { if alreadyDownloaded(a.cfg, resp, a.destPath) { return nil diff --git a/pkg/http/download_test.go b/pkg/http/download_test.go index c0c44c570..b34ff72ae 100644 --- a/pkg/http/download_test.go +++ b/pkg/http/download_test.go @@ -208,6 +208,24 @@ func TestDownloadToFilePermanentStatusFailsFast(t *testing.T) { } } +func TestDownloadToFileTooManyRequestsRetries(t *testing.T) { + var calls int + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + calls++ + w.WriteHeader(http.StatusTooManyRequests) + })) + defer srv.Close() + + dest := filepath.Join(t.TempDir(), "out.bin") + err := DownloadToFile(context.Background(), srv.URL, dest, fastBackoff()) + if err == nil || !strings.Contains(err.Error(), "429") { + t.Fatalf("expected a 429 error, got %v", err) + } + if calls != 3 { + t.Fatalf("expected 429 to be retried, got %d attempts", calls) + } +} + func TestDownloadToFileWithMode(t *testing.T) { srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { _, _ = w.Write([]byte("binary")) diff --git a/pkg/ide/download.go b/pkg/ide/download.go index f3f53119c..4d0d66b25 100644 --- a/pkg/ide/download.go +++ b/pkg/ide/download.go @@ -73,14 +73,26 @@ func extractToStaging( } if err := extract.Extract(file, stagingDir, opts...); err != nil { - if rmErr := os.RemoveAll(stagingDir); rmErr != nil { - log.Warnf("cleanup staging dir: path=%s err=%v", stagingDir, rmErr) - } + cleanupStaging(stagingDir) return "", fmt.Errorf("extract %s: %w", url, err) } + + // MkdirTemp created stagingDir as 0700; restore the broader install-dir + // mode that extract.Extract previously produced for destDir. + // #nosec G302 -- IDE install dir + if err := os.Chmod(stagingDir, 0o755); err != nil { + cleanupStaging(stagingDir) + return "", fmt.Errorf("set install dir mode: %w", err) + } return stagingDir, nil } +func cleanupStaging(stagingDir string) { + if rmErr := os.RemoveAll(stagingDir); rmErr != nil { + log.Warnf("cleanup staging dir: path=%s err=%v", stagingDir, rmErr) + } +} + // swapIntoPlace replaces destDir with stagingDir, preserving the previous // contents until the swap succeeds so a rename failure cannot leave destDir // missing. diff --git a/pkg/ide/download_test.go b/pkg/ide/download_test.go index ad20f1828..cace65d6b 100644 --- a/pkg/ide/download_test.go +++ b/pkg/ide/download_test.go @@ -49,6 +49,13 @@ func TestDownloadAndExtractSuccess(t *testing.T) { if _, err := os.Stat(filepath.Join(dest, "code")); err != nil { t.Fatalf("expected extracted file: %v", err) } + info, err := os.Stat(dest) + if err != nil { + t.Fatal(err) + } + if info.Mode().Perm() != 0o755 { + t.Fatalf("expected install dir mode 0755, got %v", info.Mode().Perm()) + } } func TestDownloadAndExtractCorruptArchiveReportsExtractError(t *testing.T) { From ea0782f8838d4d5c33124323df6b941b591ba8cf Mon Sep 17 00:00:00 2001 From: Samuel K Date: Thu, 23 Jul 2026 17:52:36 -0500 Subject: [PATCH 11/16] refactor(http): move stall timeout into a RoundTripper decorator The inactivity timeout is a cross-cutting transport concern, so express it as an http.RoundTripper (StallTransport) composed onto a download-scoped client rather than threading a watchdog through the download loop. The download algorithm no longer knows about stalls; it just sees a retryable ErrStalled. Kept opt-in per client so the shared client and long-poll callers are unaffected. --- pkg/http/download.go | 147 +++++++++++++---------------------------- pkg/http/stall.go | 130 ++++++++++++++++++++++++++++++++++++ pkg/http/stall_test.go | 89 +++++++++++++++++++++++++ 3 files changed, 264 insertions(+), 102 deletions(-) create mode 100644 pkg/http/stall.go create mode 100644 pkg/http/stall_test.go diff --git a/pkg/http/download.go b/pkg/http/download.go index d2a1ea554..1bafcbdf9 100644 --- a/pkg/http/download.go +++ b/pkg/http/download.go @@ -21,21 +21,44 @@ var DefaultDownloadBackoff = wait.Backoff{ Steps: 3, } -// DefaultStallTimeout bounds how long a download attempt may make no progress -// (no response headers or no bytes read) before it is aborted and retried. It +// DefaultStallTimeout bounds how long a download may make no progress (no +// response headers or no bytes read) before it is aborted and retried. It // guards against unresponsive endpoints without penalizing large downloads // that are still transferring data, since the shared HTTP client has no -// overall Client.Timeout. +// overall Client.Timeout. Enforcement lives in the download client's +// StallTransport rather than the download loop. const DefaultStallTimeout = 60 * time.Second +var ( + downloadClientOnce sync.Once + downloadClient *http.Client +) + +// defaultDownloadClient returns a shared client whose transport aborts stalled +// transfers after DefaultStallTimeout. +func defaultDownloadClient() *http.Client { + downloadClientOnce.Do(func() { downloadClient = newStallClient(DefaultStallTimeout) }) + return downloadClient +} + +// newStallClient wraps the shared HTTP client's transport with an inactivity +// timeout, preserving its connection pool and TLS configuration. +func newStallClient(timeout time.Duration) *http.Client { + base := GetHTTPClient() + return &http.Client{ + Transport: NewStallTransport(base.Transport, timeout), + CheckRedirect: base.CheckRedirect, + Jar: base.Jar, + } +} + type downloadConfig struct { - headers map[string]string - backoff wait.Backoff - client *http.Client - mode os.FileMode - wrap func(r io.Reader, totalSize int64) io.Reader - skipIfSized bool - stallTimeout time.Duration + headers map[string]string + backoff wait.Backoff + client *http.Client + mode os.FileMode + wrap func(r io.Reader, totalSize int64) io.Reader + skipIfSized bool } // DownloadOption customizes DownloadToFile. @@ -75,10 +98,12 @@ func SkipIfSameSize() DownloadOption { return func(c *downloadConfig) { c.skipIfSized = true } } -// WithStallTimeout overrides how long an attempt may make no progress before it -// is aborted. A non-positive value disables the stall guard entirely. +// WithStallTimeout overrides how long the download may make no progress before +// it is aborted, by swapping in a client with that inactivity timeout. A +// non-positive value disables the stall guard. Has no effect when combined with +// a later WithClient. func WithStallTimeout(d time.Duration) DownloadOption { - return func(c *downloadConfig) { c.stallTimeout = d } + return func(c *downloadConfig) { c.client = newStallClient(d) } } // permanentError marks a failure that will not be resolved by retrying, such as @@ -93,10 +118,9 @@ func (e *permanentError) Unwrap() error { return e.err } // transfers) with backoff and failing fast on permanent ones (other 4xx). func DownloadToFile(ctx context.Context, url, destPath string, opts ...DownloadOption) error { cfg := downloadConfig{ - backoff: DefaultDownloadBackoff, - client: GetHTTPClient(), - mode: 0o600, - stallTimeout: DefaultStallTimeout, + backoff: DefaultDownloadBackoff, + client: defaultDownloadClient(), + mode: 0o600, } for _, opt := range opts { opt(&cfg) @@ -136,26 +160,11 @@ type downloadAttempt struct { cfg *downloadConfig url string destPath string - wd *stallWatchdog } func downloadOnce(ctx context.Context, cfg *downloadConfig, url, destPath string) error { a := &downloadAttempt{cfg: cfg, url: url, destPath: destPath} - if cfg.stallTimeout > 0 { - var cancel context.CancelFunc - ctx, cancel = context.WithCancel(ctx) - defer cancel() - a.wd = newStallWatchdog(cfg.stallTimeout, cancel) - defer a.wd.stop() - } - - err := a.do(ctx) - // A stall aborts via context cancellation; surface it as a clear, retryable - // error instead of a bare "context canceled". - if a.wd != nil && a.wd.fired() && errors.Is(err, context.Canceled) { - return fmt.Errorf("download %s: stalled for %s", url, cfg.stallTimeout) - } - return err + return a.do(ctx) } func (a *downloadAttempt) do(ctx context.Context) error { @@ -173,11 +182,6 @@ func (a *downloadAttempt) do(ctx context.Context) error { } defer func() { _ = resp.Body.Close() }() - // Headers received: give the body transfer a fresh inactivity window. - if a.wd != nil { - a.wd.reset() - } - if resp.StatusCode >= http.StatusBadRequest { statusErr := fmt.Errorf("download %s: %s", a.url, resp.Status) if retryableStatus(resp.StatusCode) { @@ -259,16 +263,12 @@ func (a *downloadAttempt) streamToTempFile( } // source builds the reader copied to disk, applying the optional progress -// wrapper and stall watchdog. +// wrapper. The inactivity guard lives in the client's StallTransport. func (a *downloadAttempt) source(resp *http.Response) io.Reader { - src := io.Reader(resp.Body) if a.cfg.wrap != nil { - src = a.cfg.wrap(resp.Body, resp.ContentLength) - } - if a.wd != nil { - src = &stallResetReader{r: src, wd: a.wd} + return a.cfg.wrap(resp.Body, resp.ContentLength) } - return src + return resp.Body } // alreadyDownloaded reports whether destPath already holds the full download, @@ -290,60 +290,3 @@ func verifyContentLength(resp *http.Response, written int64, url string) error { } return nil } - -// stallWatchdog aborts a download attempt (by cancelling its context) when no -// progress is made within timeout. Each observed unit of progress reschedules -// the deadline. -type stallWatchdog struct { - timeout time.Duration - timer *time.Timer - mu sync.Mutex - triggered bool -} - -func newStallWatchdog(timeout time.Duration, cancel context.CancelFunc) *stallWatchdog { - w := &stallWatchdog{timeout: timeout} - w.timer = time.AfterFunc(timeout, func() { - w.mu.Lock() - w.triggered = true - w.mu.Unlock() - cancel() - }) - return w -} - -func (w *stallWatchdog) reset() { - w.mu.Lock() - defer w.mu.Unlock() - if w.triggered { - return - } - w.timer.Reset(w.timeout) -} - -func (w *stallWatchdog) stop() { - w.mu.Lock() - defer w.mu.Unlock() - w.timer.Stop() -} - -func (w *stallWatchdog) fired() bool { - w.mu.Lock() - defer w.mu.Unlock() - return w.triggered -} - -// stallResetReader reschedules the watchdog deadline on each successful read so -// a steadily-progressing transfer is never aborted. -type stallResetReader struct { - r io.Reader - wd *stallWatchdog -} - -func (s *stallResetReader) Read(p []byte) (int, error) { - n, err := s.r.Read(p) - if n > 0 { - s.wd.reset() - } - return n, err -} diff --git a/pkg/http/stall.go b/pkg/http/stall.go new file mode 100644 index 000000000..c7c3b1f95 --- /dev/null +++ b/pkg/http/stall.go @@ -0,0 +1,130 @@ +package http + +import ( + "context" + "errors" + "io" + "net/http" + "sync" + "time" +) + +// ErrStalled indicates a request was aborted because it made no progress — +// neither response headers nor body bytes — within the configured timeout. +var ErrStalled = errors.New("stalled: no progress within timeout") + +// StallTransport wraps a RoundTripper so a request is aborted when it makes no +// progress within timeout: no response headers after the request is sent, or no +// body bytes read while streaming. A steadily-progressing transfer keeps +// rescheduling the deadline, so this bounds unresponsive endpoints without +// penalizing large downloads. +// +// It is deliberately opt-in per client: an inactivity timeout is unsafe for +// intentionally-idle responses such as long-poll or watch streams, so it must +// not be applied to the shared general-purpose client. +type StallTransport struct { + base http.RoundTripper + timeout time.Duration +} + +// NewStallTransport returns base wrapped with an inactivity timeout. A +// non-positive timeout returns base unchanged. +func NewStallTransport(base http.RoundTripper, timeout time.Duration) http.RoundTripper { + if base == nil { + base = http.DefaultTransport + } + if timeout <= 0 { + return base + } + return &StallTransport{base: base, timeout: timeout} +} + +func (t *StallTransport) RoundTrip(req *http.Request) (*http.Response, error) { + ctx, cancel := context.WithCancel(req.Context()) + wd := newStallWatchdog(t.timeout, cancel) + + resp, err := t.base.RoundTrip(req.WithContext(ctx)) + if err != nil { + wd.stop() + cancel() + if wd.fired() { + return nil, ErrStalled + } + return nil, err + } + + // Headers received: give the body transfer a fresh inactivity window and + // tie the watchdog's lifetime to the response body. + wd.reset() + resp.Body = &stallBody{rc: resp.Body, wd: wd, cancel: cancel} + return resp, nil +} + +// stallBody reschedules the watchdog on each read and translates a +// watchdog-triggered cancellation into ErrStalled. Closing it stops the +// watchdog and releases the derived context. +type stallBody struct { + rc io.ReadCloser + wd *stallWatchdog + cancel context.CancelFunc +} + +func (b *stallBody) Read(p []byte) (int, error) { + n, err := b.rc.Read(p) + if n > 0 { + b.wd.reset() + } + // Once the watchdog has fired the context is cancelled, so any resulting + // (non-EOF) read error is attributable to the stall. + if err != nil && !errors.Is(err, io.EOF) && b.wd.fired() { + return n, ErrStalled + } + return n, err +} + +func (b *stallBody) Close() error { + b.wd.stop() + b.cancel() + return b.rc.Close() +} + +// stallWatchdog cancels a request (via cancel) when no progress is made within +// timeout. Each observed unit of progress reschedules the deadline. +type stallWatchdog struct { + timeout time.Duration + timer *time.Timer + mu sync.Mutex + triggered bool +} + +func newStallWatchdog(timeout time.Duration, cancel context.CancelFunc) *stallWatchdog { + w := &stallWatchdog{timeout: timeout} + w.timer = time.AfterFunc(timeout, func() { + w.mu.Lock() + w.triggered = true + w.mu.Unlock() + cancel() + }) + return w +} + +func (w *stallWatchdog) reset() { + w.mu.Lock() + defer w.mu.Unlock() + if w.triggered { + return + } + w.timer.Reset(w.timeout) +} + +func (w *stallWatchdog) stop() { + w.mu.Lock() + defer w.mu.Unlock() + w.timer.Stop() +} + +func (w *stallWatchdog) fired() bool { + w.mu.Lock() + defer w.mu.Unlock() + return w.triggered +} diff --git a/pkg/http/stall_test.go b/pkg/http/stall_test.go new file mode 100644 index 000000000..a175576ea --- /dev/null +++ b/pkg/http/stall_test.go @@ -0,0 +1,89 @@ +package http + +import ( + "errors" + "io" + "net/http" + "net/http/httptest" + "testing" + "time" +) + +func TestNewStallTransportPassthroughWhenDisabled(t *testing.T) { + base := http.DefaultTransport + if got := NewStallTransport(base, 0); got != base { + t.Fatalf("expected base transport returned unchanged when timeout <= 0") + } + if _, ok := NewStallTransport(base, time.Second).(*StallTransport); !ok { + t.Fatalf("expected a *StallTransport when timeout > 0") + } +} + +func TestStallTransportAbortsIdleBody(t *testing.T) { + release := make(chan struct{}) + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + w.(http.Flusher).Flush() + _, _ = w.Write([]byte("partial")) + w.(http.Flusher).Flush() + select { + case <-r.Context().Done(): + case <-release: + } + })) + defer srv.Close() + defer close(release) + + client := &http.Client{Transport: NewStallTransport(http.DefaultTransport, 50*time.Millisecond)} + resp, err := client.Get(srv.URL) + if err != nil { + t.Fatalf("Get: %v", err) + } + defer func() { _ = resp.Body.Close() }() + + _, err = io.ReadAll(resp.Body) + if !errors.Is(err, ErrStalled) { + t.Fatalf("expected ErrStalled reading a stalled body, got %v", err) + } +} + +func TestStallTransportAllowsProgressingBody(t *testing.T) { + const body = "steadily-delivered-payload" + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte(body)) + })) + defer srv.Close() + + client := &http.Client{Transport: NewStallTransport(http.DefaultTransport, 50*time.Millisecond)} + resp, err := client.Get(srv.URL) + if err != nil { + t.Fatalf("Get: %v", err) + } + defer func() { _ = resp.Body.Close() }() + + got, err := io.ReadAll(resp.Body) + if err != nil { + t.Fatalf("expected clean read, got %v", err) + } + if string(got) != body { + t.Fatalf("body mismatch: got %q want %q", got, body) + } +} + +func TestStallTransportHeaderStall(t *testing.T) { + release := make(chan struct{}) + srv := httptest.NewServer(http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) { + select { + case <-r.Context().Done(): + case <-release: + } + })) + defer srv.Close() + defer close(release) + + client := &http.Client{Transport: NewStallTransport(http.DefaultTransport, 50*time.Millisecond)} + _, err := client.Get(srv.URL) + if !errors.Is(err, ErrStalled) { + t.Fatalf("expected ErrStalled when headers never arrive, got %v", err) + } +} From d0be3db6dcfab22f3c60d9edfce3d14640209135 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Thu, 23 Jul 2026 18:03:01 -0500 Subject: [PATCH 12/16] feat(http): composable client with retry, user-agent, header timeout MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Restructure the shared HTTP client as a base transport plus composable RoundTripper decorators: - baseRoundTripper: TLS/proxy/dial + ResponseHeaderTimeout (bounds the header wait for all callers). - WithUserAgent: sends devsy/ when the caller sets none. - RetryTransport: retries idempotent (GET/HEAD) requests on connection errors, 5xx and 429 with exponential backoff, honoring Retry-After. Non-idempotent methods and caller-cancelled requests are never retried, so it composes safely with existing app-level retry loops. GetHTTPClient now composes base + user-agent + retry. The download client composes base + user-agent + stall (no retry — DownloadToFile owns download retries, including truncation, so layering retry would double-retry). --- pkg/http/download.go | 16 ++-- pkg/http/http.go | 50 ++++++++-- pkg/http/retry.go | 139 ++++++++++++++++++++++++++++ pkg/http/retry_test.go | 182 +++++++++++++++++++++++++++++++++++++ pkg/http/useragent.go | 28 ++++++ pkg/http/useragent_test.go | 50 ++++++++++ 6 files changed, 450 insertions(+), 15 deletions(-) create mode 100644 pkg/http/retry.go create mode 100644 pkg/http/retry_test.go create mode 100644 pkg/http/useragent.go create mode 100644 pkg/http/useragent_test.go diff --git a/pkg/http/download.go b/pkg/http/download.go index 1bafcbdf9..81426a1e7 100644 --- a/pkg/http/download.go +++ b/pkg/http/download.go @@ -41,15 +41,15 @@ func defaultDownloadClient() *http.Client { return downloadClient } -// newStallClient wraps the shared HTTP client's transport with an inactivity -// timeout, preserving its connection pool and TLS configuration. +// newStallClient builds a download client: the shared base transport with a +// User-Agent and an inactivity timeout. It deliberately omits the retry +// decorator — DownloadToFile owns download retries (including truncation, which +// is only detectable after streaming to disk), so layering RetryTransport here +// would double-retry transient statuses. func newStallClient(timeout time.Duration) *http.Client { - base := GetHTTPClient() - return &http.Client{ - Transport: NewStallTransport(base.Transport, timeout), - CheckRedirect: base.CheckRedirect, - Jar: base.Jar, - } + rt := WithUserAgent(baseRoundTripper(), UserAgent()) + rt = NewStallTransport(rt, timeout) + return &http.Client{Transport: rt} } type downloadConfig struct { diff --git a/pkg/http/http.go b/pkg/http/http.go index 14188ad8e..e9664e715 100644 --- a/pkg/http/http.go +++ b/pkg/http/http.go @@ -6,28 +6,64 @@ import ( "os" "strconv" "sync" + "time" "github.com/devsy-org/devsy/pkg/config" "github.com/devsy-org/devsy/pkg/log" + "github.com/devsy-org/devsy/pkg/version" ) +// DefaultResponseHeaderTimeout bounds how long a request waits for response +// headers after it has been written. It bounds unresponsive endpoints without +// affecting body streaming (once headers arrive the timer no longer applies). +// It is unsuitable only for servers that deliberately withhold response headers +// until data is ready — an uncommon long-poll design. +const DefaultResponseHeaderTimeout = 60 * time.Second + var ( - httpClient *http.Client + baseTransportOnce sync.Once + baseTransportInst *http.Transport + httpClientOnce sync.Once + httpClientInst *http.Client ) -func GetHTTPClient() *http.Client { - httpClientOnce.Do(func() { - customTransport := http.DefaultTransport.(*http.Transport).Clone() +// baseRoundTripper returns the shared base transport (TLS, proxy, dial and +// header timeouts) with no higher-level behavior. Purpose-built clients compose +// decorators — user-agent, retry, stall — on top of it, sharing its connection +// pool. +func baseRoundTripper() *http.Transport { + baseTransportOnce.Do(func() { + t := http.DefaultTransport.(*http.Transport).Clone() + t.ResponseHeaderTimeout = DefaultResponseHeaderTimeout if insecureTLSEnabled() { // #nosec G402 -- enabled with DEVSY_INSECURE_TLS env - customTransport.TLSClientConfig = &tls.Config{InsecureSkipVerify: true} + t.TLSClientConfig = &tls.Config{InsecureSkipVerify: true} log.Warnf("TLS certificate verification is disabled (%s)", config.EnvInsecureTLS) } - httpClient = &http.Client{Transport: customTransport} + baseTransportInst = t + }) + return baseTransportInst +} + +// GetHTTPClient returns the shared general-purpose client. It sends a +// User-Agent and retries idempotent requests (GET, HEAD) on transient failures +// — connection errors, 5xx, and 429 — honoring the server's Retry-After hint. +// +// It is not intended for long-lived streaming responses that withhold headers; +// build a dedicated client for those. +func GetHTTPClient() *http.Client { + httpClientOnce.Do(func() { + rt := WithUserAgent(baseRoundTripper(), UserAgent()) + rt = NewRetryTransport(rt, DefaultRetry) + httpClientInst = &http.Client{Transport: rt} }) + return httpClientInst +} - return httpClient +// UserAgent is the value sent on requests made through the package's clients. +func UserAgent() string { + return config.BinaryName + "/" + version.GetVersion() } // insecureTLSEnabled reports whether TLS certificate verification should be diff --git a/pkg/http/retry.go b/pkg/http/retry.go new file mode 100644 index 000000000..bc1632916 --- /dev/null +++ b/pkg/http/retry.go @@ -0,0 +1,139 @@ +package http + +import ( + "context" + "errors" + "io" + "net/http" + "strconv" + "time" + + "github.com/devsy-org/devsy/pkg/log" +) + +// RetryConfig configures RetryTransport. +type RetryConfig struct { + MaxAttempts int // total attempts including the first (values <= 1 disable retry) + BaseDelay time.Duration // first backoff delay; doubles each attempt + MaxDelay time.Duration // cap for both backoff and honored Retry-After +} + +// DefaultRetry is the retry policy applied to the shared general-purpose client. +var DefaultRetry = RetryConfig{MaxAttempts: 3, BaseDelay: time.Second, MaxDelay: 30 * time.Second} + +// maxDrainOnRetry bounds how much of a discarded response body is read to keep +// the connection reusable before the next attempt. +const maxDrainOnRetry = 4 << 10 + +// RetryTransport retries idempotent requests (GET, HEAD) on connection errors +// and retryable statuses (5xx, 429) with exponential backoff, honoring the +// server's Retry-After hint. Non-idempotent methods and caller-cancelled +// requests are never retried, so it is safe on the shared client. +type RetryTransport struct { + base http.RoundTripper + cfg RetryConfig +} + +// NewRetryTransport wraps base with cfg. A policy of at most one attempt returns +// base unchanged. +func NewRetryTransport(base http.RoundTripper, cfg RetryConfig) http.RoundTripper { + if base == nil { + base = http.DefaultTransport + } + if cfg.MaxAttempts <= 1 { + return base + } + return &RetryTransport{base: base, cfg: cfg} +} + +func (t *RetryTransport) RoundTrip(req *http.Request) (*http.Response, error) { + if !idempotentMethod(req.Method) { + return t.base.RoundTrip(req) + } + + for attempt := 1; ; attempt++ { + resp, err := t.base.RoundTrip(req) + if attempt >= t.cfg.MaxAttempts || !t.retryable(resp, err) { + return resp, err + } + + delay := t.backoff(attempt) + if resp != nil { + if after, ok := ParseRetryAfter(resp); ok { + delay = minDuration(after, t.cfg.MaxDelay) + } + drainAndClose(resp) + } + if sleepErr := sleepContext(req.Context(), delay); sleepErr != nil { + return nil, sleepErr + } + log.Debugf("retrying request: method=%s url=%s attempt=%d", req.Method, req.URL, attempt+1) + } +} + +// retryable reports whether a failed attempt should be retried. Caller +// cancellation and deadline expiry are never retried. +func (t *RetryTransport) retryable(resp *http.Response, err error) bool { + if err != nil { + return !errors.Is(err, context.Canceled) && !errors.Is(err, context.DeadlineExceeded) + } + return resp.StatusCode >= http.StatusInternalServerError || + resp.StatusCode == http.StatusTooManyRequests +} + +func (t *RetryTransport) backoff(attempt int) time.Duration { + delay := t.cfg.BaseDelay << (attempt - 1) + return minDuration(delay, t.cfg.MaxDelay) +} + +func idempotentMethod(method string) bool { + return method == "" || method == http.MethodGet || method == http.MethodHead +} + +// ParseRetryAfter returns the delay requested by a Retry-After header, in either +// delta-seconds or HTTP-date form. Negative or malformed values report false. +func ParseRetryAfter(resp *http.Response) (time.Duration, bool) { + v := resp.Header.Get("Retry-After") + if v == "" { + return 0, false + } + if secs, err := strconv.Atoi(v); err == nil { + if secs < 0 { + return 0, false + } + return time.Duration(secs) * time.Second, true + } + if at, err := http.ParseTime(v); err == nil { + if d := time.Until(at); d > 0 { + return d, true + } + return 0, true + } + return 0, false +} + +func drainAndClose(resp *http.Response) { + _, _ = io.Copy(io.Discard, io.LimitReader(resp.Body, maxDrainOnRetry)) + _ = resp.Body.Close() +} + +func sleepContext(ctx context.Context, d time.Duration) error { + if d <= 0 { + return ctx.Err() + } + timer := time.NewTimer(d) + defer timer.Stop() + select { + case <-ctx.Done(): + return ctx.Err() + case <-timer.C: + return nil + } +} + +func minDuration(a, b time.Duration) time.Duration { + if a < b { + return a + } + return b +} diff --git a/pkg/http/retry_test.go b/pkg/http/retry_test.go new file mode 100644 index 000000000..2c4a3bcc6 --- /dev/null +++ b/pkg/http/retry_test.go @@ -0,0 +1,182 @@ +package http + +import ( + "context" + "errors" + "io" + "net/http" + "strings" + "testing" + "time" +) + +type stubRoundTripper struct { + calls int + fn func(attempt int) (*http.Response, error) +} + +func (s *stubRoundTripper) RoundTrip(*http.Request) (*http.Response, error) { + s.calls++ + return s.fn(s.calls) +} + +func stubResp(code int, hdr http.Header) *http.Response { + if hdr == nil { + hdr = http.Header{} + } + return &http.Response{ + StatusCode: code, + Header: hdr, + Body: io.NopCloser(strings.NewReader("body")), + } +} + +func fastRetry() RetryConfig { + return RetryConfig{MaxAttempts: 3, BaseDelay: time.Millisecond, MaxDelay: time.Millisecond} +} + +func mustGet(t *testing.T, method string) *http.Request { + t.Helper() + req, err := http.NewRequest(method, "http://example.test", nil) + if err != nil { + t.Fatal(err) + } + return req +} + +func TestRetryTransportRetriesServerErrorThenSucceeds(t *testing.T) { + stub := &stubRoundTripper{fn: func(attempt int) (*http.Response, error) { + if attempt < 3 { + return stubResp(http.StatusServiceUnavailable, nil), nil + } + return stubResp(http.StatusOK, nil), nil + }} + rt := NewRetryTransport(stub, fastRetry()) + + resp, err := rt.RoundTrip(mustGet(t, http.MethodGet)) + if err != nil { + t.Fatalf("RoundTrip: %v", err) + } + if resp.StatusCode != http.StatusOK { + t.Fatalf("expected 200, got %d", resp.StatusCode) + } + if stub.calls != 3 { + t.Fatalf("expected 3 attempts, got %d", stub.calls) + } +} + +func TestRetryTransportGivesUpAfterMaxAttempts(t *testing.T) { + stub := &stubRoundTripper{fn: func(int) (*http.Response, error) { + return stubResp(http.StatusBadGateway, nil), nil + }} + rt := NewRetryTransport(stub, fastRetry()) + + resp, err := rt.RoundTrip(mustGet(t, http.MethodGet)) + if err != nil { + t.Fatalf("RoundTrip: %v", err) + } + if resp.StatusCode != http.StatusBadGateway { + t.Fatalf("expected last 502 returned, got %d", resp.StatusCode) + } + if stub.calls != 3 { + t.Fatalf("expected 3 attempts, got %d", stub.calls) + } +} + +func TestRetryTransportDoesNotRetryNonIdempotent(t *testing.T) { + stub := &stubRoundTripper{fn: func(int) (*http.Response, error) { + return stubResp(http.StatusServiceUnavailable, nil), nil + }} + rt := NewRetryTransport(stub, fastRetry()) + + if _, err := rt.RoundTrip(mustGet(t, http.MethodPost)); err != nil { + t.Fatalf("RoundTrip: %v", err) + } + if stub.calls != 1 { + t.Fatalf("expected POST not retried, got %d attempts", stub.calls) + } +} + +func TestRetryTransportDoesNotRetryCancelledContext(t *testing.T) { + stub := &stubRoundTripper{fn: func(int) (*http.Response, error) { + return nil, context.Canceled + }} + rt := NewRetryTransport(stub, fastRetry()) + + _, err := rt.RoundTrip(mustGet(t, http.MethodGet)) + if !errors.Is(err, context.Canceled) { + t.Fatalf("expected context.Canceled, got %v", err) + } + if stub.calls != 1 { + t.Fatalf("expected no retry on cancellation, got %d attempts", stub.calls) + } +} + +func TestRetryTransportHonorsRetryAfterAndContext(t *testing.T) { + stub := &stubRoundTripper{fn: func(int) (*http.Response, error) { + return stubResp(http.StatusTooManyRequests, http.Header{"Retry-After": {"3600"}}), nil + }} + // Large MaxDelay so the honored Retry-After (1h) is the delay; a short + // context deadline proves the sleep is context-aware. + rt := NewRetryTransport( + stub, + RetryConfig{MaxAttempts: 3, BaseDelay: time.Second, MaxDelay: time.Hour}, + ) + + ctx, cancel := context.WithTimeout(context.Background(), 20*time.Millisecond) + defer cancel() + _, err := rt.RoundTrip(mustGet(t, http.MethodGet).WithContext(ctx)) + if !errors.Is(err, context.DeadlineExceeded) { + t.Fatalf("expected deadline exceeded while honoring Retry-After, got %v", err) + } + if stub.calls != 1 { + t.Fatalf("expected to block on Retry-After, got %d attempts", stub.calls) + } +} + +func TestNewRetryTransportDisabledReturnsBase(t *testing.T) { + base := http.DefaultTransport + if got := NewRetryTransport(base, RetryConfig{MaxAttempts: 1}); got != base { + t.Fatalf("expected base returned when retry disabled") + } +} + +func TestParseRetryAfter(t *testing.T) { + tests := []struct { + name string + value string + wantOK bool + want time.Duration + }{ + {"seconds", "5", true, 5 * time.Second}, + {"zero", "0", true, 0}, + {"negative", "-1", false, 0}, + {"garbage", "soon", false, 0}, + {"missing", "", false, 0}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + hdr := http.Header{} + if tt.value != "" { + hdr.Set("Retry-After", tt.value) + } + got, ok := ParseRetryAfter(&http.Response{Header: hdr}) + if ok != tt.wantOK { + t.Fatalf("ok = %v, want %v", ok, tt.wantOK) + } + if ok && got != tt.want { + t.Fatalf("duration = %v, want %v", got, tt.want) + } + }) + } +} + +func TestParseRetryAfterHTTPDate(t *testing.T) { + future := time.Now().Add(2 * time.Hour).UTC().Format(http.TimeFormat) + hdr := http.Header{} + hdr.Set("Retry-After", future) + got, ok := ParseRetryAfter(&http.Response{Header: hdr}) + if !ok || got <= 0 { + t.Fatalf("expected a positive delay for a future date, got %v ok=%v", got, ok) + } +} diff --git a/pkg/http/useragent.go b/pkg/http/useragent.go new file mode 100644 index 000000000..c552c4705 --- /dev/null +++ b/pkg/http/useragent.go @@ -0,0 +1,28 @@ +package http + +import "net/http" + +// WithUserAgent wraps base so requests that don't already carry a User-Agent +// header get the given value. An empty agent returns base unchanged. +func WithUserAgent(base http.RoundTripper, agent string) http.RoundTripper { + if base == nil { + base = http.DefaultTransport + } + if agent == "" { + return base + } + return &userAgentTransport{base: base, agent: agent} +} + +type userAgentTransport struct { + base http.RoundTripper + agent string +} + +func (t *userAgentTransport) RoundTrip(req *http.Request) (*http.Response, error) { + if req.Header.Get("User-Agent") == "" { + req = req.Clone(req.Context()) + req.Header.Set("User-Agent", t.agent) + } + return t.base.RoundTrip(req) +} diff --git a/pkg/http/useragent_test.go b/pkg/http/useragent_test.go new file mode 100644 index 000000000..db7ac0169 --- /dev/null +++ b/pkg/http/useragent_test.go @@ -0,0 +1,50 @@ +package http + +import ( + "net/http" + "testing" +) + +type recordingRoundTripper struct{ lastUA string } + +func (r *recordingRoundTripper) RoundTrip(req *http.Request) (*http.Response, error) { + r.lastUA = req.Header.Get("User-Agent") + return &http.Response{StatusCode: http.StatusOK, Body: http.NoBody}, nil +} + +func TestWithUserAgentSetsHeaderWhenAbsent(t *testing.T) { + rec := &recordingRoundTripper{} + rt := WithUserAgent(rec, "devsy/test") + + req, _ := http.NewRequest(http.MethodGet, "http://example.test", nil) + if _, err := rt.RoundTrip(req); err != nil { + t.Fatal(err) + } + if rec.lastUA != "devsy/test" { + t.Fatalf("expected user-agent set, got %q", rec.lastUA) + } + if req.Header.Get("User-Agent") != "" { + t.Fatalf("original request must not be mutated, got %q", req.Header.Get("User-Agent")) + } +} + +func TestWithUserAgentPreservesExisting(t *testing.T) { + rec := &recordingRoundTripper{} + rt := WithUserAgent(rec, "devsy/test") + + req, _ := http.NewRequest(http.MethodGet, "http://example.test", nil) + req.Header.Set("User-Agent", "caller/1.0") + if _, err := rt.RoundTrip(req); err != nil { + t.Fatal(err) + } + if rec.lastUA != "caller/1.0" { + t.Fatalf("expected caller user-agent preserved, got %q", rec.lastUA) + } +} + +func TestWithUserAgentEmptyReturnsBase(t *testing.T) { + base := http.DefaultTransport + if got := WithUserAgent(base, ""); got != base { + t.Fatalf("expected base returned for empty agent") + } +} From 5641859831a68e3cc0491c185a9af249ef7b0043 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Thu, 23 Jul 2026 18:08:44 -0500 Subject: [PATCH 13/16] docs(http): trim comments to match self-documenting style --- pkg/http/download.go | 42 +++++++----------------------------------- pkg/http/http.go | 18 +++--------------- pkg/http/retry.go | 23 ++++++----------------- pkg/http/stall.go | 24 ++---------------------- pkg/ide/download.go | 24 ++++++++---------------- 5 files changed, 26 insertions(+), 105 deletions(-) diff --git a/pkg/http/download.go b/pkg/http/download.go index 81426a1e7..6bccd501e 100644 --- a/pkg/http/download.go +++ b/pkg/http/download.go @@ -21,12 +21,6 @@ var DefaultDownloadBackoff = wait.Backoff{ Steps: 3, } -// DefaultStallTimeout bounds how long a download may make no progress (no -// response headers or no bytes read) before it is aborted and retried. It -// guards against unresponsive endpoints without penalizing large downloads -// that are still transferring data, since the shared HTTP client has no -// overall Client.Timeout. Enforcement lives in the download client's -// StallTransport rather than the download loop. const DefaultStallTimeout = 60 * time.Second var ( @@ -34,18 +28,13 @@ var ( downloadClient *http.Client ) -// defaultDownloadClient returns a shared client whose transport aborts stalled -// transfers after DefaultStallTimeout. func defaultDownloadClient() *http.Client { downloadClientOnce.Do(func() { downloadClient = newStallClient(DefaultStallTimeout) }) return downloadClient } -// newStallClient builds a download client: the shared base transport with a -// User-Agent and an inactivity timeout. It deliberately omits the retry -// decorator — DownloadToFile owns download retries (including truncation, which -// is only detectable after streaming to disk), so layering RetryTransport here -// would double-retry transient statuses. +// newStallClient builds a download client with a stall timeout but no retry: +// DownloadToFile owns download retries (including truncation). func newStallClient(timeout time.Duration) *http.Client { rt := WithUserAgent(baseRoundTripper(), UserAgent()) rt = NewStallTransport(rt, timeout) @@ -98,24 +87,19 @@ func SkipIfSameSize() DownloadOption { return func(c *downloadConfig) { c.skipIfSized = true } } -// WithStallTimeout overrides how long the download may make no progress before -// it is aborted, by swapping in a client with that inactivity timeout. A -// non-positive value disables the stall guard. Has no effect when combined with -// a later WithClient. +// WithStallTimeout swaps in a client with the given inactivity timeout. func WithStallTimeout(d time.Duration) DownloadOption { return func(c *downloadConfig) { c.client = newStallClient(d) } } -// permanentError marks a failure that will not be resolved by retrying, such as -// a 4xx response or a malformed request. +// permanentError marks a failure that retrying will not resolve. type permanentError struct{ err error } func (e *permanentError) Error() string { return e.err.Error() } func (e *permanentError) Unwrap() error { return e.err } // DownloadToFile downloads url into destPath, retrying transient failures -// (connection errors, 5xx responses, 429 Too Many Requests, and truncated -// transfers) with backoff and failing fast on permanent ones (other 4xx). +// (connection errors, 5xx, 429, truncated transfers) and failing fast on 4xx. func DownloadToFile(ctx context.Context, url, destPath string, opts ...DownloadOption) error { cfg := downloadConfig{ backoff: DefaultDownloadBackoff, @@ -154,8 +138,6 @@ func isPermanent(err error) bool { return errors.As(err, &perr) } -// downloadAttempt carries the per-attempt state so helpers stay within the -// project's argument limit. type downloadAttempt struct { cfg *downloadConfig url string @@ -193,9 +175,6 @@ func (a *downloadAttempt) do(ctx context.Context) error { return a.copyToFile(resp) } -// retryableStatus reports whether an HTTP error status should be retried. 5xx -// responses are transient, and 429 Too Many Requests is an explicit -// retry-later signal; all other 4xx statuses are treated as permanent. func retryableStatus(code int) bool { return code >= http.StatusInternalServerError || code == http.StatusTooManyRequests } @@ -212,9 +191,8 @@ func (a *downloadAttempt) copyToFile(resp *http.Response) error { return &permanentError{fmt.Errorf("create download folder: %w", err)} } - // Download to a temporary file in the destination directory and rename it - // into place only on success, so a failed transfer never leaves a partial - // file at destPath (which callers may mistake for a complete download). + // Write to a temp file and rename on success so a failed transfer never + // leaves a partial file at destPath. tmpPath, err := a.streamToTempFile(resp, destDir, destPath) if err != nil { return err @@ -226,8 +204,6 @@ func (a *downloadAttempt) copyToFile(resp *http.Response) error { return nil } -// streamToTempFile writes the response body to a temporary file next to -// destPath and returns its path on success, removing it on any failure. func (a *downloadAttempt) streamToTempFile( resp *http.Response, destDir, destPath string, ) (string, error) { @@ -262,8 +238,6 @@ func (a *downloadAttempt) streamToTempFile( return tmpPath, nil } -// source builds the reader copied to disk, applying the optional progress -// wrapper. The inactivity guard lives in the client's StallTransport. func (a *downloadAttempt) source(resp *http.Response) io.Reader { if a.cfg.wrap != nil { return a.cfg.wrap(resp.Body, resp.ContentLength) @@ -271,8 +245,6 @@ func (a *downloadAttempt) source(resp *http.Response) io.Reader { return resp.Body } -// alreadyDownloaded reports whether destPath already holds the full download, -// based on a size match against the advertised Content-Length. func alreadyDownloaded(cfg *downloadConfig, resp *http.Response, destPath string) bool { if !cfg.skipIfSized || resp.ContentLength < 0 { return false diff --git a/pkg/http/http.go b/pkg/http/http.go index e9664e715..fe81a6d8a 100644 --- a/pkg/http/http.go +++ b/pkg/http/http.go @@ -13,11 +13,6 @@ import ( "github.com/devsy-org/devsy/pkg/version" ) -// DefaultResponseHeaderTimeout bounds how long a request waits for response -// headers after it has been written. It bounds unresponsive endpoints without -// affecting body streaming (once headers arrive the timer no longer applies). -// It is unsuitable only for servers that deliberately withhold response headers -// until data is ready — an uncommon long-poll design. const DefaultResponseHeaderTimeout = 60 * time.Second var ( @@ -28,10 +23,7 @@ var ( httpClientInst *http.Client ) -// baseRoundTripper returns the shared base transport (TLS, proxy, dial and -// header timeouts) with no higher-level behavior. Purpose-built clients compose -// decorators — user-agent, retry, stall — on top of it, sharing its connection -// pool. +// baseRoundTripper is the shared transport that purpose-built clients wrap. func baseRoundTripper() *http.Transport { baseTransportOnce.Do(func() { t := http.DefaultTransport.(*http.Transport).Clone() @@ -46,12 +38,8 @@ func baseRoundTripper() *http.Transport { return baseTransportInst } -// GetHTTPClient returns the shared general-purpose client. It sends a -// User-Agent and retries idempotent requests (GET, HEAD) on transient failures -// — connection errors, 5xx, and 429 — honoring the server's Retry-After hint. -// -// It is not intended for long-lived streaming responses that withhold headers; -// build a dedicated client for those. +// GetHTTPClient returns the shared client: base transport, user-agent, and +// retry of idempotent requests. Not for streaming responses that withhold headers. func GetHTTPClient() *http.Client { httpClientOnce.Do(func() { rt := WithUserAgent(baseRoundTripper(), UserAgent()) diff --git a/pkg/http/retry.go b/pkg/http/retry.go index bc1632916..d2528dd7c 100644 --- a/pkg/http/retry.go +++ b/pkg/http/retry.go @@ -11,31 +11,23 @@ import ( "github.com/devsy-org/devsy/pkg/log" ) -// RetryConfig configures RetryTransport. type RetryConfig struct { - MaxAttempts int // total attempts including the first (values <= 1 disable retry) - BaseDelay time.Duration // first backoff delay; doubles each attempt - MaxDelay time.Duration // cap for both backoff and honored Retry-After + MaxAttempts int // <=1 disables retry + BaseDelay time.Duration + MaxDelay time.Duration } -// DefaultRetry is the retry policy applied to the shared general-purpose client. var DefaultRetry = RetryConfig{MaxAttempts: 3, BaseDelay: time.Second, MaxDelay: 30 * time.Second} -// maxDrainOnRetry bounds how much of a discarded response body is read to keep -// the connection reusable before the next attempt. const maxDrainOnRetry = 4 << 10 -// RetryTransport retries idempotent requests (GET, HEAD) on connection errors -// and retryable statuses (5xx, 429) with exponential backoff, honoring the -// server's Retry-After hint. Non-idempotent methods and caller-cancelled -// requests are never retried, so it is safe on the shared client. +// RetryTransport retries idempotent requests on connection errors, 5xx and 429, +// honoring Retry-After. Non-idempotent and cancelled requests pass through. type RetryTransport struct { base http.RoundTripper cfg RetryConfig } -// NewRetryTransport wraps base with cfg. A policy of at most one attempt returns -// base unchanged. func NewRetryTransport(base http.RoundTripper, cfg RetryConfig) http.RoundTripper { if base == nil { base = http.DefaultTransport @@ -71,8 +63,6 @@ func (t *RetryTransport) RoundTrip(req *http.Request) (*http.Response, error) { } } -// retryable reports whether a failed attempt should be retried. Caller -// cancellation and deadline expiry are never retried. func (t *RetryTransport) retryable(resp *http.Response, err error) bool { if err != nil { return !errors.Is(err, context.Canceled) && !errors.Is(err, context.DeadlineExceeded) @@ -90,8 +80,7 @@ func idempotentMethod(method string) bool { return method == "" || method == http.MethodGet || method == http.MethodHead } -// ParseRetryAfter returns the delay requested by a Retry-After header, in either -// delta-seconds or HTTP-date form. Negative or malformed values report false. +// ParseRetryAfter reads a Retry-After header in delta-seconds or HTTP-date form. func ParseRetryAfter(resp *http.Response) (time.Duration, bool) { v := resp.Header.Get("Retry-After") if v == "" { diff --git a/pkg/http/stall.go b/pkg/http/stall.go index c7c3b1f95..0ec9f1df3 100644 --- a/pkg/http/stall.go +++ b/pkg/http/stall.go @@ -9,26 +9,15 @@ import ( "time" ) -// ErrStalled indicates a request was aborted because it made no progress — -// neither response headers nor body bytes — within the configured timeout. var ErrStalled = errors.New("stalled: no progress within timeout") -// StallTransport wraps a RoundTripper so a request is aborted when it makes no -// progress within timeout: no response headers after the request is sent, or no -// body bytes read while streaming. A steadily-progressing transfer keeps -// rescheduling the deadline, so this bounds unresponsive endpoints without -// penalizing large downloads. -// -// It is deliberately opt-in per client: an inactivity timeout is unsafe for -// intentionally-idle responses such as long-poll or watch streams, so it must -// not be applied to the shared general-purpose client. +// StallTransport aborts a request that makes no progress within timeout. +// Unsafe for intentionally-idle responses (long-poll, watch streams). type StallTransport struct { base http.RoundTripper timeout time.Duration } -// NewStallTransport returns base wrapped with an inactivity timeout. A -// non-positive timeout returns base unchanged. func NewStallTransport(base http.RoundTripper, timeout time.Duration) http.RoundTripper { if base == nil { base = http.DefaultTransport @@ -53,16 +42,11 @@ func (t *StallTransport) RoundTrip(req *http.Request) (*http.Response, error) { return nil, err } - // Headers received: give the body transfer a fresh inactivity window and - // tie the watchdog's lifetime to the response body. wd.reset() resp.Body = &stallBody{rc: resp.Body, wd: wd, cancel: cancel} return resp, nil } -// stallBody reschedules the watchdog on each read and translates a -// watchdog-triggered cancellation into ErrStalled. Closing it stops the -// watchdog and releases the derived context. type stallBody struct { rc io.ReadCloser wd *stallWatchdog @@ -74,8 +58,6 @@ func (b *stallBody) Read(p []byte) (int, error) { if n > 0 { b.wd.reset() } - // Once the watchdog has fired the context is cancelled, so any resulting - // (non-EOF) read error is attributable to the stall. if err != nil && !errors.Is(err, io.EOF) && b.wd.fired() { return n, ErrStalled } @@ -88,8 +70,6 @@ func (b *stallBody) Close() error { return b.rc.Close() } -// stallWatchdog cancels a request (via cancel) when no progress is made within -// timeout. Each observed unit of progress reschedules the deadline. type stallWatchdog struct { timeout time.Duration timer *time.Timer diff --git a/pkg/ide/download.go b/pkg/ide/download.go index 4d0d66b25..eb14378b5 100644 --- a/pkg/ide/download.go +++ b/pkg/ide/download.go @@ -11,10 +11,9 @@ import ( "github.com/devsy-org/devsy/pkg/log" ) -// DownloadAndExtract downloads the archive at url to a temporary file, then -// extracts it into destDir. The archive is unpacked into a sibling staging -// directory and swapped into place only on success, so a failed download or -// extraction never destroys a pre-existing install. +// DownloadAndExtract downloads the archive at url and extracts it into destDir. +// It stages into a sibling dir and swaps on success, so a failure leaves any +// pre-existing install intact. func DownloadAndExtract( ctx context.Context, url, destDir string, @@ -33,8 +32,6 @@ func DownloadAndExtract( return swapIntoPlace(stagingDir, destDir) } -// downloadToTemp downloads url into a temporary file and returns its path. The -// temporary file is removed on failure. func downloadToTemp(ctx context.Context, url string) (string, error) { tmpFile, err := os.CreateTemp("", "devsy-ide-*.tar.gz") if err != nil { @@ -50,9 +47,6 @@ func downloadToTemp(ctx context.Context, url string) (string, error) { return tmpPath, nil } -// extractToStaging unpacks the archive at tmpPath into a sibling staging -// directory of destDir and returns its path. The staging directory is removed -// if extraction fails. func extractToStaging( url, tmpPath, destDir string, opts ...extract.Option, @@ -77,8 +71,7 @@ func extractToStaging( return "", fmt.Errorf("extract %s: %w", url, err) } - // MkdirTemp created stagingDir as 0700; restore the broader install-dir - // mode that extract.Extract previously produced for destDir. + // MkdirTemp yields 0700; restore the 0755 install-dir mode. // #nosec G302 -- IDE install dir if err := os.Chmod(stagingDir, 0o755); err != nil { cleanupStaging(stagingDir) @@ -93,9 +86,8 @@ func cleanupStaging(stagingDir string) { } } -// swapIntoPlace replaces destDir with stagingDir, preserving the previous -// contents until the swap succeeds so a rename failure cannot leave destDir -// missing. +// swapIntoPlace replaces destDir with stagingDir, keeping the old contents +// until the swap succeeds. func swapIntoPlace(stagingDir, destDir string) error { backupDir := stagingDir + ".old" hasBackup, err := stashExisting(destDir, backupDir) @@ -122,8 +114,8 @@ func swapIntoPlace(stagingDir, destDir string) error { return nil } -// stashExisting moves an existing destDir aside to backupDir so it can be -// restored if the subsequent swap fails. It reports whether a backup was made. +// stashExisting moves an existing destDir aside so it can be restored if the +// swap fails; it reports whether a backup was made. func stashExisting(destDir, backupDir string) (bool, error) { _, err := os.Stat(destDir) if os.IsNotExist(err) { From 9e8fe960be4841c678511d87932e051e582bf5a0 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Thu, 23 Jul 2026 18:25:37 -0500 Subject: [PATCH 14/16] fix(http): guard retry against unreplayable request bodies MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Retry only idempotent, bodyless requests — a consumed body cannot be replayed. Add redirect and body-guard regression tests. --- pkg/http/retry.go | 11 ++++++++--- pkg/http/retry_test.go | 19 +++++++++++++++++++ pkg/http/stall_test.go | 25 +++++++++++++++++++++++++ 3 files changed, 52 insertions(+), 3 deletions(-) diff --git a/pkg/http/retry.go b/pkg/http/retry.go index d2528dd7c..30819d2b3 100644 --- a/pkg/http/retry.go +++ b/pkg/http/retry.go @@ -39,7 +39,7 @@ func NewRetryTransport(base http.RoundTripper, cfg RetryConfig) http.RoundTrippe } func (t *RetryTransport) RoundTrip(req *http.Request) (*http.Response, error) { - if !idempotentMethod(req.Method) { + if !retriableRequest(req) { return t.base.RoundTrip(req) } @@ -76,8 +76,13 @@ func (t *RetryTransport) backoff(attempt int) time.Duration { return minDuration(delay, t.cfg.MaxDelay) } -func idempotentMethod(method string) bool { - return method == "" || method == http.MethodGet || method == http.MethodHead +// retriableRequest allows retry only for idempotent, bodyless requests; a +// consumed request body cannot be safely replayed. +func retriableRequest(req *http.Request) bool { + if req.Body != nil && req.Body != http.NoBody { + return false + } + return req.Method == "" || req.Method == http.MethodGet || req.Method == http.MethodHead } // ParseRetryAfter reads a Retry-After header in delta-seconds or HTTP-date form. diff --git a/pkg/http/retry_test.go b/pkg/http/retry_test.go index 2c4a3bcc6..412cbbaa5 100644 --- a/pkg/http/retry_test.go +++ b/pkg/http/retry_test.go @@ -97,6 +97,25 @@ func TestRetryTransportDoesNotRetryNonIdempotent(t *testing.T) { } } +func TestRetryTransportDoesNotRetryRequestWithBody(t *testing.T) { + stub := &stubRoundTripper{fn: func(int) (*http.Response, error) { + return stubResp(http.StatusServiceUnavailable, nil), nil + }} + rt := NewRetryTransport(stub, fastRetry()) + + req, err := http.NewRequest(http.MethodGet, "http://example.test", strings.NewReader("x")) + if err != nil { + t.Fatal(err) + } + req.GetBody = nil + if _, err := rt.RoundTrip(req); err != nil { + t.Fatalf("RoundTrip: %v", err) + } + if stub.calls != 1 { + t.Fatalf("expected no retry for a request with a body, got %d attempts", stub.calls) + } +} + func TestRetryTransportDoesNotRetryCancelledContext(t *testing.T) { stub := &stubRoundTripper{fn: func(int) (*http.Response, error) { return nil, context.Canceled diff --git a/pkg/http/stall_test.go b/pkg/http/stall_test.go index a175576ea..3747229c3 100644 --- a/pkg/http/stall_test.go +++ b/pkg/http/stall_test.go @@ -70,6 +70,31 @@ func TestStallTransportAllowsProgressingBody(t *testing.T) { } } +func TestStallTransportFollowsRedirect(t *testing.T) { + origin := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte("final-content")) + })) + defer origin.Close() + redir := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + http.Redirect(w, r, origin.URL, http.StatusFound) + })) + defer redir.Close() + + client := &http.Client{Transport: NewStallTransport(http.DefaultTransport, 5*time.Second)} + resp, err := client.Get(redir.URL) + if err != nil { + t.Fatalf("Get through redirect: %v", err) + } + defer func() { _ = resp.Body.Close() }() + body, err := io.ReadAll(resp.Body) + if err != nil { + t.Fatalf("read body: %v", err) + } + if string(body) != "final-content" { + t.Fatalf("got %q", body) + } +} + func TestStallTransportHeaderStall(t *testing.T) { release := make(chan struct{}) srv := httptest.NewServer(http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) { From 297d417cf381ede179d10832e87b493272d3887e Mon Sep 17 00:00:00 2001 From: Samuel K Date: Thu, 23 Jul 2026 18:33:22 -0500 Subject: [PATCH 15/16] fix(download): harden install swap and retry backoff - swapIntoPlace: if restore also fails after a failed swap, return an error naming the preserved backup path instead of silently warning, so a rare double-rename failure cannot lose the install. - RetryTransport.backoff: clamp shift overflow to MaxDelay. --- pkg/http/retry.go | 3 +++ pkg/http/retry_test.go | 9 +++++++++ pkg/ide/download.go | 5 ++++- 3 files changed, 16 insertions(+), 1 deletion(-) diff --git a/pkg/http/retry.go b/pkg/http/retry.go index 30819d2b3..1e13f6e63 100644 --- a/pkg/http/retry.go +++ b/pkg/http/retry.go @@ -73,6 +73,9 @@ func (t *RetryTransport) retryable(resp *http.Response, err error) bool { func (t *RetryTransport) backoff(attempt int) time.Duration { delay := t.cfg.BaseDelay << (attempt - 1) + if delay <= 0 { // shift overflow + return t.cfg.MaxDelay + } return minDuration(delay, t.cfg.MaxDelay) } diff --git a/pkg/http/retry_test.go b/pkg/http/retry_test.go index 412cbbaa5..351fe45a2 100644 --- a/pkg/http/retry_test.go +++ b/pkg/http/retry_test.go @@ -153,6 +153,15 @@ func TestRetryTransportHonorsRetryAfterAndContext(t *testing.T) { } } +func TestRetryTransportBackoffOverflowClampsToMax(t *testing.T) { + rt := &RetryTransport{ + cfg: RetryConfig{MaxAttempts: 100, BaseDelay: time.Second, MaxDelay: 30 * time.Second}, + } + if got := rt.backoff(64); got != rt.cfg.MaxDelay { + t.Fatalf("expected overflowed backoff to clamp to MaxDelay, got %v", got) + } +} + func TestNewRetryTransportDisabledReturnsBase(t *testing.T) { base := http.DefaultTransport if got := NewRetryTransport(base, RetryConfig{MaxAttempts: 1}); got != base { diff --git a/pkg/ide/download.go b/pkg/ide/download.go index eb14378b5..32117abfa 100644 --- a/pkg/ide/download.go +++ b/pkg/ide/download.go @@ -100,7 +100,10 @@ func swapIntoPlace(stagingDir, destDir string) error { _ = os.RemoveAll(stagingDir) if hasBackup { if restoreErr := os.Rename(backupDir, destDir); restoreErr != nil { - log.Warnf("restore install dir: path=%s err=%v", destDir, restoreErr) + return fmt.Errorf( + "install to %s failed (%w); restore failed (%v); prior install at %s", + destDir, err, restoreErr, backupDir, + ) } } return fmt.Errorf("install to %s: %w", destDir, err) From 11fa6e3ca5299a2cfa292fc680a8e03932cca9cc Mon Sep 17 00:00:00 2001 From: Samuel K Date: Thu, 23 Jul 2026 19:37:28 -0500 Subject: [PATCH 16/16] fix(http): propagate cancellation and guard shared client body reads - DownloadToFile: on interruption return the context error, not the stale last transient error, so callers can detect cancellation. - GetHTTPClient: replace the base ResponseHeaderTimeout with a StallTransport (idle guard covering header and body) so a stalled body read cannot hang a caller, without a total timeout that would break large downloads. Removes the header-timeout/stall-timeout overlap. --- pkg/http/download.go | 3 +++ pkg/http/download_test.go | 20 ++++++++++++++++++++ pkg/http/http.go | 12 +++++------- 3 files changed, 28 insertions(+), 7 deletions(-) diff --git a/pkg/http/download.go b/pkg/http/download.go index 6bccd501e..1ab43b501 100644 --- a/pkg/http/download.go +++ b/pkg/http/download.go @@ -128,6 +128,9 @@ func DownloadToFile(ctx context.Context, url, destPath string, opts ...DownloadO }, ) if wait.Interrupted(err) { + if ctxErr := ctx.Err(); ctxErr != nil { + return ctxErr + } return lastErr } return err diff --git a/pkg/http/download_test.go b/pkg/http/download_test.go index b34ff72ae..52eb9f1e4 100644 --- a/pkg/http/download_test.go +++ b/pkg/http/download_test.go @@ -2,6 +2,7 @@ package http import ( "context" + "errors" "io" "net/http" "net/http/httptest" @@ -163,6 +164,25 @@ func TestDownloadToFileStallTimeoutAborts(t *testing.T) { } } +func TestDownloadToFileReportsContextCancellation(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusServiceUnavailable) + })) + defer srv.Close() + + // Backoff longer than the context deadline, so cancellation happens during + // the wait after a transient (non-context) failure. + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Millisecond) + defer cancel() + slow := WithBackoff(wait.Backoff{Duration: 100 * time.Millisecond, Factor: 1, Steps: 3}) + + dest := filepath.Join(t.TempDir(), "out.bin") + err := DownloadToFile(ctx, srv.URL, dest, slow) + if !errors.Is(err, context.DeadlineExceeded) { + t.Fatalf("expected context deadline error, got %v", err) + } +} + func TestDownloadToFileRecoversAfterTransientFailure(t *testing.T) { const body = "recovered-payload" var calls int diff --git a/pkg/http/http.go b/pkg/http/http.go index fe81a6d8a..c665501fa 100644 --- a/pkg/http/http.go +++ b/pkg/http/http.go @@ -6,15 +6,12 @@ import ( "os" "strconv" "sync" - "time" "github.com/devsy-org/devsy/pkg/config" "github.com/devsy-org/devsy/pkg/log" "github.com/devsy-org/devsy/pkg/version" ) -const DefaultResponseHeaderTimeout = 60 * time.Second - var ( baseTransportOnce sync.Once baseTransportInst *http.Transport @@ -27,7 +24,6 @@ var ( func baseRoundTripper() *http.Transport { baseTransportOnce.Do(func() { t := http.DefaultTransport.(*http.Transport).Clone() - t.ResponseHeaderTimeout = DefaultResponseHeaderTimeout if insecureTLSEnabled() { // #nosec G402 -- enabled with DEVSY_INSECURE_TLS env t.TLSClientConfig = &tls.Config{InsecureSkipVerify: true} @@ -38,11 +34,13 @@ func baseRoundTripper() *http.Transport { return baseTransportInst } -// GetHTTPClient returns the shared client: base transport, user-agent, and -// retry of idempotent requests. Not for streaming responses that withhold headers. +// GetHTTPClient returns the shared client: base transport, stall guard, +// user-agent, and retry of idempotent requests. The stall guard bounds idle +// header/body waits, so it must not be used for intentionally-idle streams. func GetHTTPClient() *http.Client { httpClientOnce.Do(func() { - rt := WithUserAgent(baseRoundTripper(), UserAgent()) + rt := NewStallTransport(baseRoundTripper(), DefaultStallTimeout) + rt = WithUserAgent(rt, UserAgent()) rt = NewRetryTransport(rt, DefaultRetry) httpClientInst = &http.Client{Transport: rt} })