fix(http): EOF downloads for VSCode CLI#725
Merged
Merged
Conversation
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.
✅ Deploy Preview for devsydev canceled.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds shared HTTP download, retry, stall-detection, and archive-extraction utilities. Devcontainer features and IDE installers now use these utilities with configurable TLS, headers, caching, progress reporting, file modes, and staged installation replacement. ChangesDownload infrastructure
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Installer
participant DownloadAndExtract
participant DownloadToFile
participant ArchiveStaging
Installer->>DownloadAndExtract: request archive installation
DownloadAndExtract->>DownloadToFile: download archive to temporary file
DownloadToFile-->>DownloadAndExtract: completed archive
DownloadAndExtract->>ArchiveStaging: extract and stage files
ArchiveStaging-->>DownloadAndExtract: extraction result
DownloadAndExtract-->>Installer: installed destination or error
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
✅ Deploy Preview for images-devsy-sh canceled.
|
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.
skevetter
changed the title
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.
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.
…ition 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).
skevetter
marked this pull request as ready for review
July 23, 2026 06:29
skevetter
changed the title
Signed-off-by: Samuel K <skevetter@pm.me>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/ide/fleet/fleet.go (1)
64-91: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRemove truncated Fleet files before retrying the install.
DownloadToFiledoes not deletedestPathwhen all retries orStartBackgroundOncefail; it can leave a partialfleetBinarybecause retry attempts write withO_TRUNCto the final destination. SinceInstall()only usesos.Stat(fleetBinary)to detect an existing installation, a partial file from a failed previous run will make the next run try to execute and start a corrupt binary. Return on download error, but remove/reinstate the file if it no longer exists or is invalid before starting.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/ide/fleet/fleet.go` around lines 64 - 91, Update FleetServer.Install around the devsyhttp.DownloadToFile call to remove fleetBinary when downloading fails, before returning the error, so truncated files cannot be mistaken for an installed binary. Also validate the existing fleetBinary detected by os.Stat before calling Start, removing it when missing or invalid and continuing through the download path.
🧹 Nitpick comments (1)
pkg/devcontainer/feature/features.go (1)
548-554: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPropagate a bounded context into
DownloadToFilecallers. These downloads usecontext.Background()from the caller all the way intohttp.NewRequestWithContext, whileGetHTTPClient()does not set anhttp.Client.Timeout, so an unresponsive endpoint can block an in-flight download and stall the caller indefinitely. Thread a caller-owned context with a deadline into the integration wrapper functions instead ofcontext.Background().🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/devcontainer/feature/features.go` around lines 548 - 554, Replace context.Background() with caller-owned, deadline-bounded contexts in the DownloadToFile wrapper flows, threading each context through the relevant integration functions. Update pkg/devcontainer/feature/features.go (the feature download function), pkg/ide/fleet/fleet.go (the Fleet download caller), pkg/ide/jetbrains/generic.go (the JetBrains download caller), and pkg/ide/rstudio/rstudio.go (the RStudio download caller); preserve existing download behavior while ensuring every request has a finite deadline.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/http/download.go`:
- Around line 139-164: The copyToFile function must write downloads atomically:
create a temporary file in destPath’s directory, apply cfg.mode, copy and close
the download, verify the content length, then rename the temp file to destPath
only on success. Ensure all failure paths remove the temporary file, and
preserve cfg.mode even when destPath already exists.
In `@pkg/ide/download.go`:
- Around line 39-42: Update the extraction flow in the download function
containing extract.Extract so archives are unpacked into a sibling staging
directory rather than the caller-provided destDir, and only replace destDir
after extraction succeeds. On extraction failure, clean up staging without
removing pre-existing destination contents, and add a regression test that seeds
destDir before downloading an invalid archive and verifies it remains intact.
---
Outside diff comments:
In `@pkg/ide/fleet/fleet.go`:
- Around line 64-91: Update FleetServer.Install around the
devsyhttp.DownloadToFile call to remove fleetBinary when downloading fails,
before returning the error, so truncated files cannot be mistaken for an
installed binary. Also validate the existing fleetBinary detected by os.Stat
before calling Start, removing it when missing or invalid and continuing through
the download path.
---
Nitpick comments:
In `@pkg/devcontainer/feature/features.go`:
- Around line 548-554: Replace context.Background() with caller-owned,
deadline-bounded contexts in the DownloadToFile wrapper flows, threading each
context through the relevant integration functions. Update
pkg/devcontainer/feature/features.go (the feature download function),
pkg/ide/fleet/fleet.go (the Fleet download caller), pkg/ide/jetbrains/generic.go
(the JetBrains download caller), and pkg/ide/rstudio/rstudio.go (the RStudio
download caller); preserve existing download behavior while ensuring every
request has a finite deadline.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5fb61b84-a3da-4bea-a3ed-4deed7f6eac5
📒 Files selected for processing (14)
pkg/config/env.gopkg/devcontainer/feature/features.gopkg/http/download.gopkg/http/download_test.gopkg/http/http.gopkg/http/http_test.gopkg/ide/codeserver/codeserver.gopkg/ide/download.gopkg/ide/download_test.gopkg/ide/fleet/fleet.gopkg/ide/jetbrains/generic.gopkg/ide/openvscode/openvscode.gopkg/ide/rstudio/rstudio.gopkg/ide/vscodeweb/vscodeweb.go
- 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.
skevetter
marked this pull request as draft
July 23, 2026 20:18
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.
- 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.
skevetter
marked this pull request as ready for review
July 23, 2026 21:19
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/http/download.go`:
- Around line 181-187: Update downloadOnce’s HTTP status handling so 429 Too
Many Requests does not return permanentError and instead follows the retryable
error path used for 5xx responses. Adjust the nearby documentation describing
permanent 4xx statuses to explicitly exclude retryable client statuses such as
429, while preserving permanent handling for other applicable 4xx responses.
In `@pkg/ide/download.go`:
- Around line 66-82: Update the staging/install flow used by swapIntoPlace so
the directory installed at destDir has mode 0755 after extraction and rename.
Normalize permissions after the swap succeeds, or normalize stagingDir before
renaming while preserving cleanup and error propagation for chmod failures.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 39a7c8ec-6b00-488f-8fbf-855a210d9c51
📒 Files selected for processing (4)
pkg/http/download.gopkg/http/download_test.gopkg/ide/download.gopkg/ide/download_test.go
skevetter
marked this pull request as draft
July 23, 2026 21:33
- 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.
skevetter
marked this pull request as ready for review
July 23, 2026 22:33
skevetter
marked this pull request as draft
July 23, 2026 22:44
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.
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/<version> 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).
Retry only idempotent, bodyless requests — a consumed body cannot be replayed. Add redirect and body-guard regression tests.
- 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.
skevetter
marked this pull request as ready for review
July 23, 2026 23:57
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/http/download.go (1)
113-134: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPropagate context cancellation errors instead of returning the last transient download error
wait.Interrupted(err)matches both step exhaustion and context cancellations/deadlines. Since the condition runs before each backoff sleep, a cancellation during backoff can leavelastErrholding the previous transient error, which breaks caller cancellation paths such aserrors.Is(err, context.Canceled)and shutdown behavior.🐛 Proposed fix
if wait.Interrupted(err) { + if ctxErr := ctx.Err(); ctxErr != nil { + return ctxErr + } return lastErr } return err🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/http/download.go` around lines 113 - 134, Update the retry result handling after wait.ExponentialBackoffWithContext in the download flow: when wait.Interrupted(err) indicates context cancellation or deadline expiration, return the context error from ctx rather than the stale lastErr. Preserve returning lastErr for ordinary backoff exhaustion and keep direct propagation of non-interrupted errors unchanged.
🧹 Nitpick comments (1)
pkg/http/http.go (1)
41-50: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
GetHTTPClient()has no overall request timeout.
ResponseHeaderTimeoutonly bounds the wait for response headers; once headers arrive, a slow/stalled body read on this general-purpose shared client can block the caller's goroutine indefinitely (noClient.Timeout, no stall guard like the download client has). Since this is the package's general shared client, consider setting a boundedTimeout(or clearly documenting that every caller must supply a context deadline).♻️ Proposed fix
func GetHTTPClient() *http.Client { httpClientOnce.Do(func() { rt := WithUserAgent(baseRoundTripper(), UserAgent()) rt = NewRetryTransport(rt, DefaultRetry) - httpClientInst = &http.Client{Transport: rt} + httpClientInst = &http.Client{Transport: rt, Timeout: 2 * time.Minute} }) return httpClientInst }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/http/http.go` around lines 41 - 50, Update GetHTTPClient to configure a bounded overall request timeout on the shared http.Client, covering both response-header waits and stalled body reads. Reuse the package’s established timeout constant or configuration if available, while preserving the existing transport and retry setup.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/http/http.go`:
- Around line 18-24: Update the transport caching around baseTransportOnce and
baseRoundTripper so changes to DEVSY_INSECURE_TLS are reflected after
initialization. Rebuild the cached base transport when the environment setting
changes, or provide a documented reset mechanism that tests and integrations can
invoke; ensure GetHTTPClient uses the transport matching the current TLS mode.
- Around line 26-39: Remove the fixed ResponseHeaderTimeout from the transport
returned by baseRoundTripper, or otherwise ensure download clients created
through WithStallTimeout use a transport whose header timeout is not shorter
than the configured stall timeout. Update both pkg/http/http.go at lines 26-39
and pkg/http/download.go at lines 36-42, preserving StallTransport as the
effective timeout for waits longer than 60 seconds.
---
Outside diff comments:
In `@pkg/http/download.go`:
- Around line 113-134: Update the retry result handling after
wait.ExponentialBackoffWithContext in the download flow: when
wait.Interrupted(err) indicates context cancellation or deadline expiration,
return the context error from ctx rather than the stale lastErr. Preserve
returning lastErr for ordinary backoff exhaustion and keep direct propagation of
non-interrupted errors unchanged.
---
Nitpick comments:
In `@pkg/http/http.go`:
- Around line 41-50: Update GetHTTPClient to configure a bounded overall request
timeout on the shared http.Client, covering both response-header waits and
stalled body reads. Reuse the package’s established timeout constant or
configuration if available, while preserving the existing transport and retry
setup.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 47dc8f4e-3112-416e-9c86-3bbaf15c3bb2
📒 Files selected for processing (9)
pkg/http/download.gopkg/http/http.gopkg/http/retry.gopkg/http/retry_test.gopkg/http/stall.gopkg/http/stall_test.gopkg/http/useragent.gopkg/http/useragent_test.gopkg/ide/download.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/ide/download.go
Comment on lines
18
to
24
| var ( | ||
| httpClient *http.Client | ||
| baseTransportOnce sync.Once | ||
| baseTransportInst *http.Transport | ||
|
|
||
| httpClientOnce sync.Once | ||
| httpClientInst *http.Client | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether pkg/http/http_test.go resets the transport singleton between
# TLS-related test cases (e.g. via an exported reset helper or t.Setenv ordering).
rg -n "baseTransportOnce|httpClientOnce|DEVSY_INSECURE_TLS|t.Setenv" pkg/http/http_test.goRepository: devsy-org/devsy
Length of output: 201
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate files =="
fd -a 'http.go|http_test.go' . | sed 's#^\./##'
echo "== package outline =="
ast-grep outline pkg/http/http.go || true
ast-grep outline pkg/http/http_test.go || true
echo "== relevant source =="
cat -n pkg/http/http.go
echo "== relevant tests =="
cat -n pkg/http/http_test.go
echo "== env constant references =="
rg -n "EnvInsecureTLS|DEVSY_INSECURE_TLS|baseTransportOnce|httpClientOnce|baseRoundTripper|GetHTTPClient" .Repository: devsy-org/devsy
Length of output: 6726
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== reset/override occurrences =="
rg -n "Reset|ResetHTTP|baseTransportInst|BaseTransportInst|baseTransportOnce\." .
echo "== deterministic Go sync.Once behavior probe (using stdlib only) =="
cat > /tmp/syn_once_probe.go <<'EOF'
package main
import (
"fmt"
"os"
"runtime"
"sync"
"strconv"
)
var (
envOnce sync.Once
gotValue bool
)
func getValue() string {
if !gotValue {
envOnce.Do(func() {
gotValue = true
})
}
return os.Getenv("TEST_VALUE")
}
func main() {
got = "first"
os.Setenv("TEST_VALUE", "first")
v1 := getValue()
os.Setenv("TEST_VALUE", "second")
v2 := getValue()
fmt.Printf("first=%q second=%q runtime=%s\n", v1, v2, runtime.Version())
}
EOF
go run /tmp/syn_once_probe.goRepository: devsy-org/devsy
Length of output: 9870
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== deterministic Go sync.Once behavior probe (fixed) =="
cat > /tmp/syn_once_probe.go <<'EOF'
package main
import (
"fmt"
"os"
"runtime"
"sync"
)
var (
envChecked sync.Once
checkedOnce bool
)
func getValue() string {
envChecked.Do(func() { checkedOnce = true })
return os.Getenv("TEST_VALUE")
}
func main() {
os.Setenv("TEST_VALUE", "first")
v1 := getValue()
os.Setenv("TEST_VALUE", "second")
v2 := getValue()
fmt.Printf("first=%q second=%q checked_once=%v runtime=%s\n", v1, v2, checkedOnce, runtime.Version())
}
EOF
go run /tmp/syn_once_probe.go
echo "== reset helper for transport/http client existence =="
rg -n "Reset.*HTTP|ResetHTTP|baseTransportInst|httpClientInst|baseTransportOnce|httpClientOnce" pkg/http pkg/download pkg/credentials pkg/gitsshsigning || trueRepository: devsy-org/devsy
Length of output: 827
Don’t cache DEVSY_INSECURE_TLS behind sync.Once.
baseTransportOnce.Do reads the env var once and stores that transport forever, so changing the var after baseRoundTripper() or GetHTTPClient() has run does nothing. This makes the flag order-dependent and can silently produce insecure or secure clients in tests/integration workflows. Either make the TLS mode mutable/resettable with a documented reset helper, or rebuild the singleton when the setting changes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/http/http.go` around lines 18 - 24, Update the transport caching around
baseTransportOnce and baseRoundTripper so changes to DEVSY_INSECURE_TLS are
reflected after initialization. Rebuild the cached base transport when the
environment setting changes, or provide a documented reset mechanism that tests
and integrations can invoke; ensure GetHTTPClient uses the transport matching
the current TLS mode.
skevetter
marked this pull request as draft
July 24, 2026 00:14
- 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.
skevetter
marked this pull request as ready for review
July 24, 2026 01:11
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
HTTP download hardening for
devsy up: fixes flaky VS Code CLI failures, extracts the download mechanics into a reusable abstraction, and closes a TLS-verification hole in the shared client.1. VS Code CLI
unexpected EOF(root cause)vscodewebstreamed the HTTP body straight into the extractor with only a status check. A truncated transfer still returns200; the short body only failed inside gzip/tar asunexpected EOF— misattributed to extraction, with no retry, so one hiccup failed the wholeup.2. New
devsyhttp.DownloadToFileabstractionDownloadToFile(ctx, url, destPath, opts...):WithHeaders,WithBackoff,WithClient.5xx, and truncated transfers retry with exponential backoff;4xxfails fast.Content-Length→ clearincomplete transfer, got X of Y bytesinstead of a later decode failure.Why not in the shared
*http.Client: a truncated body isn't visible atRoundTriptime (it happens while the caller streamsresp.Body), so a retrying transport can't catch it without buffering every response for the singleton client every caller shares. Completeness is a download-helper concern; domain integrity (sha256/OCI digest) stays with the caller that knows the expected value.Both duplicated paths moved onto it:
vscodeweb.downloadAndExtractdelegates download (retry + completeness), owns only extraction — an extract failure now means a genuinely corrupt archive.feature.downloadFeatureFromURLdrops its hand-rolled retry loop andtryDownload.3. TLS verification on by default
The shared client set
InsecureSkipVerifyunconditionally, disabling certificate verification for every caller — including IDE/agent binary downloads from public CDNs (MITM exposure). The genuinely-insecure paths (self-signed pro/platform, in-container setup) already use their own dedicated insecure transports, and the localhost credential flows are plain HTTP, so the shared client never needed it. Now verifies by default; insecurity is opt-in viaDEVSY_INSECURE_TLSfor environments that must reach self-signed hosts.Tests
pkg/http: download success, headers, truncation-retry, transient recovery,4xxfail-fast,5xxretry;insecureTLSEnableddefault-secure / opt-in matrix.pkg/ide/vscodeweb: download+extract success, and a complete-but-corrupt archive reported as an extract error.Summary by CodeRabbit
Retry-After) plus request user-agent injection and stall detection.DEVSY_INSECURE_TLS(logs a warning when enabled).