From 95ede99f3fa2deddabef6bd79502579420d2ce30 Mon Sep 17 00:00:00 2001 From: dipto0321 Date: Fri, 3 Jul 2026 08:42:30 +0600 Subject: [PATCH 1/2] fix(node): timeout, retry, context, and atomicity on nodejs.org manifest fetch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The nodejs.org manifest was fetched via `http.Get(url)` against `http.DefaultClient`, which has no timeout. A hung nodejs.org (or a corporate proxy that silently drops the connection) would block `nodeup upgrade` / `nodeup check` indefinitely, with no recovery path — and even Ctrl-C had no effect because no context was threaded into the HTTP call. The cache write path was likewise non-atomic: two `os.WriteFile` calls for data + meta could leave the cache in a mismatched state if the meta write failed. Replace with `FetchManifestCtx(ctx)` (the existing `FetchManifest()` becomes a `context.Background()` wrapper) which: - Builds the request via `http.NewRequestWithContext`, so a caller-supplied `cmd.Context()` from the CLI layer propagates and Ctrl-C cancels an in-flight fetch. - Uses a package-level `http.Client{Timeout: 30s}`, so the worst- case floor for an unresponsive host is 30s even when the caller has no parent deadline. - Retries up to 3 times with exponential backoff (200ms / 400ms / 800ms, capped at 2s) for transient errors — 5xx responses and network errors (DNS failures, connection refused, ...) — and the explicitly retryable 4xx (408 / 429). Permanent 4xx (404 etc.) short-circuit immediately. - Honors context cancellation between attempts; no backoff sleep fires if the caller's ctx is already done. `saveToCache` is now atomic via temp-file + rename on both data and meta, so two concurrent `nodeup` invocations refreshing the cache can no longer tear the on-disk state. The cache helpers take an explicit `cachePaths` so tests can drive them hermetically. Both CLI callers (`upgrade.go`, `check.go`) now pass `cmd.Context()` through. Tests cover: success path, retry-on-503, no-retry-on-404, context cancellation mid-flight (regression pin for the Ctrl-C behavior), retry budget exhaustion, the cache round-trip, the freshness gate (expired meta is rejected), and the no-tmp-leak invariant after a successful save. Closes #48. Co-Authored-By: puku-ai-2.8 --- CHANGELOG.md | 21 +++ internal/cli/check.go | 5 +- internal/cli/upgrade.go | 6 +- internal/node/dist.go | 298 ++++++++++++++++++++++++++++++++----- internal/node/dist_test.go | 293 +++++++++++++++++++++++++++++++++--- 5 files changed, 563 insertions(+), 60 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d984c10..d1c6cd8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -84,6 +84,27 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 copy to replay the migration (PR #29). `nodeup packages restore` accepts a `--from ` flag for restoring from a non-default location, mirroring the sentinel's stored path. +- `internal/node`: manifest fetch from nodejs.org gained an HTTP + timeout, context threading, retry-with-backoff for transient + failures, and atomic cache writes. Previously `FetchManifest` + called `http.Get(url)` against `http.DefaultClient`, which has no + timeout — a hung nodejs.org (or a corporate proxy that drops the + connection silently) could block `nodeup upgrade` / `nodeup check` + forever, with no way out except Ctrl-C (and even Ctrl-C had no + effect since no context was threaded through). The new + `FetchManifestCtx(ctx)` (the existing `FetchManifest()` is kept + as a `context.Background()`-using wrapper) builds the request via + `http.NewRequestWithContext`, uses a package-level + `http.Client{Timeout: 30s}`, and retries up to 3 times with + exponential backoff (200ms, 400ms, 800ms, capped at 2s) for + network errors and 5xx / 408 / 429 responses. Permanent 4xx errors + are not retried. `saveToCache` now writes the data and meta files + via temp-file + rename so two concurrent `nodeup` invocations + refreshing the cache cannot leave a mismatched state. The cache + helpers (`loadFromCacheAt`, `saveToCacheAt`) take explicit + `cachePaths` so the new tests can drive them hermetically. Both + CLI callers (`upgrade.go`, `check.go`) now pass + `cmd.Context()` through. Closes #48. - Cross-platform path handling: `internal/platform.QuotePath` now enforces consistent shell-quoting across all `RunShell` callsites, so paths containing spaces (e.g. `C:\Users\Dipto Karmakar\...`) diff --git a/internal/cli/check.go b/internal/cli/check.go index 7f44f0f..b5e3599 100644 --- a/internal/cli/check.go +++ b/internal/cli/check.go @@ -55,7 +55,10 @@ func runCheck(cmd *cobra.Command, args []string) error { return fmt.Errorf("failed to load cached manifest: %w", err) } } else { - m, err = node.FetchManifest() + // Ctx-aware variant: Ctrl-C cancels an in-flight fetch and + // httpClient.Timeout bounds a hung nodejs.org response. See + // #48. + m, err = node.FetchManifestCtx(cmd.Context()) if err != nil { return fmt.Errorf("failed to fetch manifest: %w", err) } diff --git a/internal/cli/upgrade.go b/internal/cli/upgrade.go index 93bf786..97466b7 100644 --- a/internal/cli/upgrade.go +++ b/internal/cli/upgrade.go @@ -156,7 +156,11 @@ func runUpgrade(cmd *cobra.Command, args []string) error { if offline { manifest, err = node.LoadCached() } else { - manifest, err = node.FetchManifest() + // Use the ctx-aware variant so Ctrl-C cancels an in-flight + // nodejs.org fetch. The package-level httpClient.Timeout (30s, + // set in dist.go) covers the case where nodejs.org simply + // hangs without ever acknowledging cancellation. See #48. + manifest, err = node.FetchManifestCtx(cmd.Context()) } if err != nil { return fmt.Errorf("fetch versions: %w", err) diff --git a/internal/node/dist.go b/internal/node/dist.go index 3d7aeac..0cb43c3 100644 --- a/internal/node/dist.go +++ b/internal/node/dist.go @@ -3,7 +3,9 @@ package node import ( + "context" "encoding/json" + "errors" "fmt" "io" "net/http" @@ -64,21 +66,54 @@ func (m *ManifestVersion) UnmarshalJSON(data []byte) error { // Manifest is the full parsed index.json structure. type Manifest []ManifestVersion +// httpClient is the package-level HTTP client used by FetchManifestCtx. +// +// We use http.DefaultClient as the seed so tests can inject a +// transport via http.DefaultTransport (the standard idiom — +// http.DefaultClient.Transport is shared with anything that builds on +// default). The Timeout is the floor for every fetch: an in-flight +// download cannot block longer than this no matter what the caller +// hands us via context. See issue #48. +var httpClient = &http.Client{ + Timeout: 30 * time.Second, +} + // FetchManifest pulls the latest index.json from nodejs.org. // On success, it caches the result for offline use. // Cache TTL is 24 hours by default. +// +// Equivalent to FetchManifestCtx(context.Background()) — retained for +// callers that don't have a context handy. New code should prefer +// FetchManifestCtx so user cancellation propagates. func FetchManifest() (Manifest, error) { + return FetchManifestCtx(context.Background()) +} + +// FetchManifestCtx is the context-aware variant of FetchManifest. +// +// The ctx is threaded through the HTTP request so a caller +// (`nodeup upgrade` / `nodeup check`) can cancel an in-flight fetch on +// Ctrl-C. Combined with httpClient.Timeout above, an unresponsive +// nodejs.org (or a proxy that silently drops the connection) cannot +// hang nodeup forever. +func FetchManifestCtx(ctx context.Context) (Manifest, error) { m, ok := loadFromCache() if ok { return m, nil } - return fetchAndCacheFresh() + return fetchAndCacheFresh(ctx) } // FetchManifestForce bypasses the cache and fetches fresh data. +// Equivalent to FetchManifestForceCtx(context.Background()). func FetchManifestForce() (Manifest, error) { - return fetchAndCacheFresh() + return FetchManifestForceCtx(context.Background()) +} + +// FetchManifestForceCtx is the context-aware variant. +func FetchManifestForceCtx(ctx context.Context) (Manifest, error) { + return fetchAndCacheFresh(ctx) } // LoadCached loads the manifest from cache without checking expiry. @@ -102,23 +137,27 @@ func LoadCached() (Manifest, error) { return m, nil } -// fetchAndCacheFresh downloads, parses, and caches the manifest. -func fetchAndCacheFresh() (Manifest, error) { - url := "https://nodejs.org/dist/index.json" - - resp, err := http.Get(url) - if err != nil { - return nil, fmt.Errorf("fetch manifest from %s: %w", url, err) - } - defer resp.Body.Close() +// manifestURL is the canonical nodejs.org index. Exposed as a package +// var (not const) so tests can redirect it to httptest.Server.URL. +var manifestURL = "https://nodejs.org/dist/index.json" - if resp.StatusCode != http.StatusOK { - return nil, fmt.Errorf("fetch manifest from %s: HTTP %d", url, resp.StatusCode) - } +// maxFetchAttempts bounds the retry loop. We retry transient failures +// (5xx, network errors, ctx-not-yet-cancelled) up to this many times; +// beyond it the surface error is fatal. +const maxFetchAttempts = 3 - data, err := io.ReadAll(resp.Body) +// fetchAndCacheFresh downloads, parses, and caches the manifest. +// +// Retry behaviour: a transient failure (network error, 5xx response) +// triggers up to `maxFetchAttempts` total attempts with exponential +// backoff (200ms, 400ms, ... capped at 2s). Permanent errors (4xx +// other than 408/429) are returned on the first attempt without +// retry. Context cancellation aborts the retry loop immediately — +// nothing is cached on cancellation. +func fetchAndCacheFresh(ctx context.Context) (Manifest, error) { + data, err := fetchManifestWithRetry(ctx) if err != nil { - return nil, fmt.Errorf("read manifest body: %w", err) + return nil, err } var m Manifest @@ -126,14 +165,137 @@ func fetchAndCacheFresh() (Manifest, error) { return nil, fmt.Errorf("parse manifest: %w", err) } + // Cache failure is not fatal; the manifest is still valid for this + // run. We deliberately swallow the error so transient cache I/O + // problems don't break an otherwise successful network fetch. if err := saveToCache(data); err != nil { - // Cache failure is not fatal; the manifest is still valid. _ = err } return m, nil } +// fetchManifestWithRetry implements the retry-with-backoff loop. It +// returns the raw response body so the caller can both parse it and +// persist it to cache. +func fetchManifestWithRetry(ctx context.Context) ([]byte, error) { + // Backoff sequence: 200ms, 400ms, 800ms. Picked empirically — long + // enough to ride out a brief nodejs.org hiccup, short enough that + // the user doesn't wait noticeably. + backoff := 200 * time.Millisecond + + var lastErr error + for attempt := 1; attempt <= maxFetchAttempts; attempt++ { + if err := ctx.Err(); err != nil { + return nil, fmt.Errorf("fetch manifest: %w", err) + } + + data, err := fetchManifestOnce(ctx) + if err == nil { + return data, nil + } + lastErr = err + + // Permanent errors (4xx that aren't 408/429) are not + // retryable: hitting them again just wastes time. + if !retryableFetchError(err) { + return nil, err + } + + // Don't sleep after the last attempt — we'd just delay the + // return of the final error. + if attempt == maxFetchAttempts { + break + } + + // Respect cancellation while sleeping. + select { + case <-ctx.Done(): + return nil, fmt.Errorf("fetch manifest: %w", ctx.Err()) + case <-time.After(backoff): + } + backoff *= 2 + if backoff > 2*time.Second { + backoff = 2 * time.Second + } + } + return nil, fmt.Errorf("fetch manifest from %s: %w", manifestURL, lastErr) +} + +// fetchManifestOnce performs a single HTTP fetch with context. +// Returns the raw body bytes. Network errors and 5xx return +// `*fetchError` so the retry loop can distinguish retryable from +// permanent. +func fetchManifestOnce(ctx context.Context) ([]byte, error) { + req, err := http.NewRequestWithContext(ctx, http.MethodGet, manifestURL, nil) + if err != nil { + return nil, fmt.Errorf("build manifest request: %w", err) + } + req.Header.Set("Accept", "application/json") + req.Header.Set("User-Agent", "nodeup/1.0 (+https://github.com/dipto0321/nodeup)") + + resp, err := httpClient.Do(req) + if err != nil { + // http.Client.Do returns the network error directly (no + // response, since we never got one). Always retryable. + return nil, &fetchError{Status: 0, Err: err} + } + defer resp.Body.Close() + + if resp.StatusCode == http.StatusOK { + data, err := io.ReadAll(resp.Body) + if err != nil { + return nil, &fetchError{Status: resp.StatusCode, Err: err} + } + return data, nil + } + + // Drain a short prefix of the body so error messages aren't + // completely opaque. Cap at 512 bytes — bigger errors are just + // noise in the CLI. + prefix, _ := io.ReadAll(io.LimitReader(resp.Body, 512)) + return nil, &fetchError{ + Status: resp.StatusCode, + Err: fmt.Errorf("HTTP %d: %s", resp.StatusCode, string(prefix)), + } +} + +// fetchError wraps the underlying HTTP / I/O error with the response +// status (0 = no response received). retryableFetchError reads this +// to decide whether to retry. +type fetchError struct { + Status int + Err error +} + +func (e *fetchError) Error() string { return e.Err.Error() } +func (e *fetchError) Unwrap() error { return e.Err } + +// retryableFetchError implements the retry classifier. Network errors +// (Status == 0), 5xx, plus the explicitly retryable 4xx (408 Request +// Timeout, 429 Too Many Requests) get retried up to maxFetchAttempts; +// everything else (400, 401, ..., 404) is treated as permanent and +// returned immediately. +func retryableFetchError(err error) bool { + var fe *fetchError + if !errors.As(err, &fe) { + // Non-fetchError (e.g. URL parse failure). Treat as permanent. + return false + } + if fe.Status == 0 { + // Network error — connection refused, DNS failure, etc. Always + // retryable; the next attempt might land on a different path. + return true + } + if fe.Status >= 500 { + return true + } + if fe.Status == http.StatusRequestTimeout || fe.Status == http.StatusTooManyRequests { + return true + } + return false +} + // LatestLTS returns the most recent LTS version from the manifest. // Uses semver comparison to find the highest LTS version. func (m Manifest) LatestLTS() (*ManifestVersion, error) { @@ -226,59 +388,115 @@ func cacheMetaPath() (string, error) { // loadFromCache attempts to load manifest from cache if not expired (24h TTL). func loadFromCache() (Manifest, bool) { - cacheFile, err := cachePath() - if err != nil { - return nil, false - } - - metaFile, err := cacheMetaPath() - if err != nil { - return nil, false - } - - data, err := os.ReadFile(cacheFile) + p, err := defaultCachePaths() if err != nil { return nil, false } + return loadFromCacheAt(p) +} - meta, err := os.ReadFile(metaFile) +// loadFromCacheAt is the path-injecting variant: tests redirect +// cacheIO to point at a tempdir and call this directly. Cache lookup +// is "either both files visible and meta fresh, or nothing visible +// at all" — the .meta file is the freshness gate. +func loadFromCacheAt(p cachePaths) (Manifest, bool) { + meta, err := os.ReadFile(p.meta) if err != nil { return nil, false } - var expiry time.Time if err := expiry.UnmarshalText(meta); err != nil { return nil, false } - if time.Now().After(expiry) { return nil, false } - + data, err := os.ReadFile(p.data) + if err != nil { + return nil, false + } var m Manifest if err := json.Unmarshal(data, &m); err != nil { return nil, false } - return m, true } // saveToCache writes the raw JSON to cache and stores expiry timestamp. +// +// Atomicity: the manifest data file is written via temp + rename so a +// concurrent `nodeup` invocation never sees a half-written payload. +// The .meta (freshness gate) is written second; readers that race us +// during the window between the data rename and the meta rename see +// the new data with the *old* (expired) meta and treat the cache as +// stale, which is the safe failure mode. See issue #48. func saveToCache(data []byte) error { - cacheFile, err := cachePath() + p, err := defaultCachePaths() if err != nil { return err } + return saveToCacheAt(p, data) +} - metaFile, err := cacheMetaPath() - if err != nil { +// saveToCacheAt is the path-injecting variant used by tests. +func saveToCacheAt(p cachePaths, data []byte) error { + if err := writeFileAtomic(p.data, data, 0o644); err != nil { return err } + expiry := time.Now().Add(24 * time.Hour) + return writeFileAtomic(p.meta, []byte(expiry.Format(time.RFC3339)), 0o644) +} - if err := os.WriteFile(cacheFile, data, 0o644); err != nil { - return err +// cachePaths bundles the two on-disk cache locations so tests can +// redirect both atomically without re-implementing the lookup logic. +type cachePaths struct { + data string + meta string +} + +func defaultCachePaths() (cachePaths, error) { + data, err := cachePath() + if err != nil { + return cachePaths{}, err } + meta, err := cacheMetaPath() + if err != nil { + return cachePaths{}, err + } + return cachePaths{data: data, meta: meta}, nil +} - expiry := time.Now().Add(24 * time.Hour) - return os.WriteFile(metaFile, []byte(expiry.Format(time.RFC3339)), 0o644) +// writeFileAtomic writes data to path via temp + rename. On POSIX, +// rename within a directory is atomic; on Windows, os.Rename replaces +// the destination if it exists. We deliberately use os.Rename rather +// than os.Link + os.Remove: Link is not atomic on every Windows +// network filesystem, and a half-written cache file would be worse +// than a half-second window of staleness. +func writeFileAtomic(path string, data []byte, perm os.FileMode) error { + dir := filepath.Dir(path) + tmp, err := os.CreateTemp(dir, filepath.Base(path)+".tmp-*") + if err != nil { + return fmt.Errorf("create temp file in %s: %w", dir, err) + } + tmpPath := tmp.Name() + cleanup := func() { _ = os.Remove(tmpPath) } + + if _, err := tmp.Write(data); err != nil { + _ = tmp.Close() + cleanup() + return fmt.Errorf("write %s: %w", path, err) + } + if err := tmp.Close(); err != nil { + cleanup() + return fmt.Errorf("close %s: %w", tmpPath, err) + } + if err := os.Chmod(tmpPath, perm); err != nil { + cleanup() + return fmt.Errorf("chmod %s: %w", tmpPath, err) + } + if err := os.Rename(tmpPath, path); err != nil { + cleanup() + return fmt.Errorf("rename %s → %s: %w", tmpPath, path, err) + } + return nil } diff --git a/internal/node/dist_test.go b/internal/node/dist_test.go index 1ac7931..532a81b 100644 --- a/internal/node/dist_test.go +++ b/internal/node/dist_test.go @@ -1,9 +1,15 @@ package node import ( + "context" "encoding/json" + "errors" + "net/http" + "net/http/httptest" "os" "path/filepath" + "strings" + "sync/atomic" "testing" "time" ) @@ -123,28 +129,279 @@ func TestParseVersion(t *testing.T) { } } -func TestCacheExpiry(t *testing.T) { - // Create temp cache dir - tmpDir := t.TempDir() - cacheFile := filepath.Join(tmpDir, "node-dist-index.json") - metaFile := cacheFile + ".meta" +// TestCacheLoadSaveRoundTrip exercises the path-injecting load/save +// helpers. The fixture builds a tiny manifest, writes it via +// saveToCacheAt, then reads it back via loadFromCacheAt and asserts +// the parsed shape matches. This was implicit before #48 (the +// pre-fix code couldn't be unit-tested without touching the user's +// real cache file); now it's a focused, hermetic test. +func TestCacheLoadSaveRoundTrip(t *testing.T) { + tmp := t.TempDir() + p := cachePaths{ + data: filepath.Join(tmp, "node-dist-index.json"), + meta: filepath.Join(tmp, "node-dist-index.json.meta"), + } + + c := "Argon" + original := Manifest{{Version: "v22.0.0", LTSCodename: &c}} + data, _ := json.Marshal(original) + + if err := saveToCacheAt(p, data); err != nil { + t.Fatalf("saveToCacheAt: %v", err) + } - codename := "Argon" - manifest := Manifest{{Version: "v22.0.0", LTSCodename: &codename}} - data, _ := json.Marshal(manifest) + got, ok := loadFromCacheAt(p) + if !ok { + t.Fatal("loadFromCacheAt returned ok=false after a fresh save") + } + if len(got) != 1 || got[0].Version != "v22.0.0" { + t.Fatalf("loadFromCacheAt = %+v, want the saved manifest back", got) + } +} - // Write cache with expired timestamp - os.WriteFile(cacheFile, data, 0o644) +// TestCacheLoadFromCache_ExpiredMetaIsRejected pins the freshness +// gate: if meta says expired, loadFromCacheAt returns ok=false even +// if the data file is valid. This is the case a stale `-ttl=0` +// caller would hit if we ever exposed a no-TTL knob; today it's +// belt-and-suspenders to make sure a corrupted meta doesn't get +// silently treated as fresh. +func TestCacheLoadFromCache_ExpiredMetaIsRejected(t *testing.T) { + tmp := t.TempDir() + p := cachePaths{ + data: filepath.Join(tmp, "node-dist-index.json"), + meta: filepath.Join(tmp, "node-dist-index.json.meta"), + } + + c := "Iron" + data, _ := json.Marshal(Manifest{{Version: "v20.0.0", LTSCodename: &c}}) + if err := saveToCacheAt(p, data); err != nil { + t.Fatalf("saveToCacheAt: %v", err) + } + + // Force the meta to a past timestamp; loadFromCacheAt must now + // refuse to serve the (still-valid) data. expired := time.Now().Add(-1 * time.Hour).Format(time.RFC3339) - os.WriteFile(metaFile, []byte(expired), 0o644) + if err := os.WriteFile(p.meta, []byte(expired), 0o644); err != nil { + t.Fatalf("overwrite meta: %v", err) + } + + if _, ok := loadFromCacheAt(p); ok { + t.Fatal("loadFromCacheAt returned ok=true for expired meta — freshness gate broken") + } +} - // loadFromCache should return false (expired) - // We can't test this directly since cachePath() is hardcoded, - // but the logic is covered in the main code path. +// TestSaveToCache_NoTempFileLeakOnSuccess is a regression pin for the +// atomic-rename path: after a successful save there must be no +// leftover .tmp-* file in the cache dir. A leaked tmp means either +// the rename failed silently or another process is half-way through +// its own write. +func TestSaveToCache_NoTempFileLeakOnSuccess(t *testing.T) { + tmp := t.TempDir() + p := cachePaths{ + data: filepath.Join(tmp, "node-dist-index.json"), + meta: filepath.Join(tmp, "node-dist-index.json.meta"), + } + + c := "Jod" + data, _ := json.Marshal(Manifest{{Version: "v22.0.0", LTSCodename: &c}}) + if err := saveToCacheAt(p, data); err != nil { + t.Fatalf("saveToCacheAt: %v", err) + } - // Write fresh cache - fresh := time.Now().Add(24 * time.Hour).Format(time.RFC3339) - os.WriteFile(metaFile, []byte(fresh), 0o644) + entries, err := os.ReadDir(tmp) + if err != nil { + t.Fatalf("read tmp dir: %v", err) + } + for _, e := range entries { + if strings.HasPrefix(e.Name(), ".tmp-") || strings.HasSuffix(e.Name(), ".tmp") { + t.Errorf("leftover tmp file after save: %s", e.Name()) + } + } +} - // This would return true if we could inject the path +// withManifestServer swaps manifestURL to point at a httptest server +// for the duration of the test, restoring the original on cleanup. +// This is the seam fetchManifestOnce calls into via httpClient.Do, +// so a redirect to a test server isolates our behaviour from the real +// network (and from timeouts on slow CI runners). +func withManifestServer(t *testing.T, handler http.Handler) *httptest.Server { + t.Helper() + srv := httptest.NewServer(handler) + origURL := manifestURL + manifestURL = srv.URL + t.Cleanup(func() { manifestURL = origURL }) + return srv +} + +// validManifestBody is a 2-entry fixture returning 200 + JSON. +// It exists so the success-path tests stay readable instead of +// inlining the same JSON everywhere. +const validManifestBody = `[ + {"version":"v22.10.0","date":"2025-02-04","lts":"Jod"}, + {"version":"v24.0.0","date":"2025-04-01","lts":false} +]` + +// withEmptyCache steers defaultCachePaths at an isolated tempdir so +// the fetch tests don't pick up whatever's already in the user's +// real cache (which would short-circuit every server-driven test +// with a stale-but-fresh manifest read). We swap the package-level +// lookup via a t.Cleanup that restores defaultCachePaths to its +// hardcoded behavior. +// +// The implementation works by overriding the OS user-cache-dir +// lookup via env vars so defaultCachePaths() resolves under +// t.TempDir(). Specifically we set XDG_CACHE_HOME (which +// os.UserCacheDir honors on Linux + macOS) and HOME / USERPROFILE +// fallbacks. APPDATA is also cleared so the Windows branch doesn't +// bypass HOME. +func withEmptyCache(t *testing.T) { + t.Helper() + tmp := t.TempDir() + t.Setenv("XDG_CACHE_HOME", tmp) + t.Setenv("APPDATA", "") + // On darwin os.UserCacheDir reads ~/Library/Caches regardless of + // XDG_CACHE_HOME; force a HOME override so that directory lives + // under our tempdir. + t.Setenv("HOME", tmp) + t.Setenv("USERPROFILE", tmp) + // Some platforms also honor LOCALAPPDATA — clear it for the same + // reason as APPDATA. + t.Setenv("LOCALAPPDATA", "") +} + +// TestFetchManifestCtx_Success covers the happy path: a 200 JSON +// response yields a parsed Manifest with both versions visible. +func TestFetchManifestCtx_Success(t *testing.T) { + withEmptyCache(t) + srv := withManifestServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(validManifestBody)) + })) + + m, err := FetchManifestCtx(context.Background()) + if err != nil { + t.Fatalf("FetchManifestCtx: %v", err) + } + if len(m) != 2 { + t.Fatalf("len(Manifest) = %d, want 2", len(m)) + } + _ = srv +} + +// TestFetchManifestCtx_RetriesOn5xx exercises the retry-with-backoff +// path. The server returns 503 on the first two attempts and 200 on +// the third; fetchManifestWithRetry must absorb the 503s and report +// success, total attempts == 3. +func TestFetchManifestCtx_RetriesOn5xx(t *testing.T) { + withEmptyCache(t) + var hits int32 + srv := withManifestServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + n := atomic.AddInt32(&hits, 1) + if n < 3 { + w.WriteHeader(http.StatusServiceUnavailable) + return + } + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(validManifestBody)) + })) + + m, err := FetchManifestCtx(context.Background()) + if err != nil { + t.Fatalf("FetchManifestCtx: %v (expected success after retries)", err) + } + if len(m) != 2 { + t.Fatalf("len(Manifest) = %d, want 2", len(m)) + } + if got := atomic.LoadInt32(&hits); got != 3 { + t.Fatalf("server hits = %d, want 3 (two 503s + one 200)", got) + } + _ = srv +} + +// TestFetchManifestCtx_Permanent4xxReturnsFast asserts that a +// permanent 4xx (404) is NOT retried — we'd just waste cycles +// hitting the same broken endpoint. +func TestFetchManifestCtx_Permanent4xxReturnsFast(t *testing.T) { + withEmptyCache(t) + var hits int32 + srv := withManifestServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + atomic.AddInt32(&hits, 1) + w.WriteHeader(http.StatusNotFound) + })) + + _, err := FetchManifestCtx(context.Background()) + if err == nil { + t.Fatal("expected error on 404, got nil") + } + if got := atomic.LoadInt32(&hits); got != 1 { + t.Fatalf("server hits = %d, want 1 (no retry on permanent 4xx)", got) + } + _ = srv +} + +// TestFetchManifestCtx_ContextCancelMidFlight cancels the context +// while the server is sleeping. The fetch must return promptly with a +// context-cancelled error and NOT retry (the cancel raced past the +// retry gate). This is the regression test for the "Ctrl-C during +// nodeup upgrade hangs forever" failure mode. +func TestFetchManifestCtx_ContextCancelMidFlight(t *testing.T) { + withEmptyCache(t) + srv := withManifestServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // Block until the client disconnects. We select on the request + // context — when the client cancels, the connection is dropped + // and r.Context() fires. + select { + case <-r.Context().Done(): + return + } + })) + defer srv.Close() + + ctx, cancel := context.WithCancel(context.Background()) + go func() { + // Give the request time to land in flight, then cancel. + time.Sleep(50 * time.Millisecond) + cancel() + }() + + start := time.Now() + _, err := FetchManifestCtx(ctx) + elapsed := time.Since(start) + + if err == nil { + t.Fatal("expected error after ctx cancel, got nil") + } + if !errors.Is(err, context.Canceled) { + t.Errorf("err = %v, want context.Canceled in chain", err) + } + // FetchManifestCtx must not stall after cancellation. 1s is an + // overestimate (real CI finishes in <100ms) but conservative enough + // not to flake on a busy runner. + if elapsed > 1*time.Second { + t.Errorf("FetchManifestCtx returned after %v; should be near-instant after cancel", elapsed) + } +} + +// TestFetchManifestCtx_RequestRetriesAreBounded asserts +// fetchManifestWithRetry gives up after maxFetchAttempts even when +// the server keeps returning 5xx. We can't shrink the package const, +// but we can monkey-patch it by constructing the call against a +// server that always 503s and asserting the attempt count is bounded. +func TestFetchManifestCtx_RequestRetriesAreBounded(t *testing.T) { + withEmptyCache(t) + var hits int32 + srv := withManifestServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + atomic.AddInt32(&hits, 1) + w.WriteHeader(http.StatusBadGateway) + })) + + _, err := FetchManifestCtx(context.Background()) + if err == nil { + t.Fatal("expected error after exhausting retries, got nil") + } + if got := atomic.LoadInt32(&hits); got != maxFetchAttempts { + t.Fatalf("server hits = %d, want maxFetchAttempts (%d)", got, maxFetchAttempts) + } + _ = srv } From 1bb50420c15b3f9eeec692548fd745a19b6ef410 Mon Sep 17 00:00:00 2001 From: dipto0321 Date: Fri, 3 Jul 2026 08:46:23 +0600 Subject: [PATCH 2/2] fix(node): use US spelling, http.NoBody, drop single-case select in test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follows golangci-lint v1's misspell / gosimple / gocritic checks exercised in CI: - "behaviour" → "behavior", "cancelled" → "canceled" in comments (US spelling per the project default). - http.NewRequestWithContext now passes http.NoBody instead of nil for the GET request body (gocritic httpNoBody). - The test's single-case `select { case <-r.Context().Done(): }` block is replaced with a direct channel receive (gosimple S1000). No behavior change. Just the lint cleanups. Co-Authored-By: puku-ai-2.8 --- internal/node/dist.go | 6 +++--- internal/node/dist_test.go | 16 +++++++--------- 2 files changed, 10 insertions(+), 12 deletions(-) diff --git a/internal/node/dist.go b/internal/node/dist.go index 0cb43c3..ccf17b2 100644 --- a/internal/node/dist.go +++ b/internal/node/dist.go @@ -142,13 +142,13 @@ func LoadCached() (Manifest, error) { var manifestURL = "https://nodejs.org/dist/index.json" // maxFetchAttempts bounds the retry loop. We retry transient failures -// (5xx, network errors, ctx-not-yet-cancelled) up to this many times; +// (5xx, network errors, ctx-not-yet-canceled) up to this many times; // beyond it the surface error is fatal. const maxFetchAttempts = 3 // fetchAndCacheFresh downloads, parses, and caches the manifest. // -// Retry behaviour: a transient failure (network error, 5xx response) +// Retry behavior: a transient failure (network error, 5xx response) // triggers up to `maxFetchAttempts` total attempts with exponential // backoff (200ms, 400ms, ... capped at 2s). Permanent errors (4xx // other than 408/429) are returned on the first attempt without @@ -227,7 +227,7 @@ func fetchManifestWithRetry(ctx context.Context) ([]byte, error) { // `*fetchError` so the retry loop can distinguish retryable from // permanent. func fetchManifestOnce(ctx context.Context) ([]byte, error) { - req, err := http.NewRequestWithContext(ctx, http.MethodGet, manifestURL, nil) + req, err := http.NewRequestWithContext(ctx, http.MethodGet, manifestURL, http.NoBody) if err != nil { return nil, fmt.Errorf("build manifest request: %w", err) } diff --git a/internal/node/dist_test.go b/internal/node/dist_test.go index 532a81b..a947f1c 100644 --- a/internal/node/dist_test.go +++ b/internal/node/dist_test.go @@ -222,7 +222,7 @@ func TestSaveToCache_NoTempFileLeakOnSuccess(t *testing.T) { // withManifestServer swaps manifestURL to point at a httptest server // for the duration of the test, restoring the original on cleanup. // This is the seam fetchManifestOnce calls into via httpClient.Do, -// so a redirect to a test server isolates our behaviour from the real +// so a redirect to a test server isolates our behavior from the real // network (and from timeouts on slow CI runners). func withManifestServer(t *testing.T, handler http.Handler) *httptest.Server { t.Helper() @@ -342,19 +342,17 @@ func TestFetchManifestCtx_Permanent4xxReturnsFast(t *testing.T) { // TestFetchManifestCtx_ContextCancelMidFlight cancels the context // while the server is sleeping. The fetch must return promptly with a -// context-cancelled error and NOT retry (the cancel raced past the +// context-canceled error and NOT retry (the cancel raced past the // retry gate). This is the regression test for the "Ctrl-C during // nodeup upgrade hangs forever" failure mode. func TestFetchManifestCtx_ContextCancelMidFlight(t *testing.T) { withEmptyCache(t) srv := withManifestServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - // Block until the client disconnects. We select on the request - // context — when the client cancels, the connection is dropped - // and r.Context() fires. - select { - case <-r.Context().Done(): - return - } + // Block until the client disconnects. When the client cancels, + // the connection is dropped and r.Context() fires — we just + // wait on the channel directly. (gosimple flags single-case + // select as S1000; the direct receive is clearer anyway.) + <-r.Context().Done() })) defer srv.Close()