From 35c21093027025d36bf4366f8506ec1b5cca34ea Mon Sep 17 00:00:00 2001 From: David Gageot Date: Wed, 29 Apr 2026 08:13:54 +0200 Subject: [PATCH 1/8] show a hint when a newer release is available Adds a best-effort GitHub-release check (cached for 24h, opt-out via DOCKER_AGENT_DISABLE_VERSION_CHECK=1) that surfaces an upgrade hint in the version subcommand and the TUI status bar. --- cmd/root/root.go | 7 + cmd/root/version.go | 8 + pkg/tui/tui.go | 2 +- pkg/tui/upgrade_hint.go | 20 ++ pkg/tui/upgrade_hint_test.go | 71 ++++++ pkg/version/check/check.go | 367 ++++++++++++++++++++++++++++++++ pkg/version/check/check_test.go | 251 ++++++++++++++++++++++ 7 files changed, 725 insertions(+), 1 deletion(-) create mode 100644 pkg/tui/upgrade_hint.go create mode 100644 pkg/tui/upgrade_hint_test.go create mode 100644 pkg/version/check/check.go create mode 100644 pkg/version/check/check_test.go diff --git a/cmd/root/root.go b/cmd/root/root.go index c670080de..a24a55c66 100644 --- a/cmd/root/root.go +++ b/cmd/root/root.go @@ -21,6 +21,7 @@ import ( "github.com/docker/docker-agent/pkg/paths" "github.com/docker/docker-agent/pkg/telemetry" "github.com/docker/docker-agent/pkg/version" + "github.com/docker/docker-agent/pkg/version/check" ) type rootFlags struct { @@ -94,6 +95,12 @@ We collect anonymous usage data to help improve docker agent. To disable: telemetry.SetGlobalTelemetryDebugMode(flags.debugMode) + // Kick off a best-effort, background check for a newer release. + // Results are cached on disk; the next invocation can surface them + // without blocking on a network call. Disabled by setting + // DOCKER_AGENT_DISABLE_VERSION_CHECK=1. + check.RefreshAsync(cmd.Context()) + if flags.enableOtel { if err := initOTelSDK(cmd.Context()); err != nil { slog.Warn("Failed to initialize OpenTelemetry SDK", "error", err) diff --git a/cmd/root/version.go b/cmd/root/version.go index 61c509f4b..7143fbeb1 100644 --- a/cmd/root/version.go +++ b/cmd/root/version.go @@ -7,6 +7,7 @@ import ( "github.com/docker/docker-agent/pkg/cli" "github.com/docker/docker-agent/pkg/telemetry" "github.com/docker/docker-agent/pkg/version" + "github.com/docker/docker-agent/pkg/version/check" ) func newVersionCmd() *cobra.Command { @@ -33,4 +34,11 @@ func runVersionCommand(cmd *cobra.Command, args []string) { } out.Printf("%s version %s\n", commandName, version.Version) out.Printf("Commit: %s\n", version.Commit) + + // Best-effort upgrade hint. Failures (offline, rate-limited, …) are + // silently ignored — the user simply does not see the line. + if res := check.Latest(cmd.Context(), version.Version); res.UpgradeAvailable() { + out.Printf("\nA newer version is available: %s\nRelease notes: https://github.com/docker/docker-agent/releases/tag/%s\n", + res.Latest, res.Latest) + } } diff --git a/pkg/tui/tui.go b/pkg/tui/tui.go index 016195a6d..d8b51c61b 100644 --- a/pkg/tui/tui.go +++ b/pkg/tui/tui.go @@ -305,7 +305,7 @@ func New(ctx context.Context, spawner SessionSpawner, initialApp *app.App, initi m.chatPage = initialChatPage // Initialize status bar (pass m as help provider) - m.statusBar = statusbar.New(m, statusbar.WithTitle(m.appName+" "+m.appVersion)) + m.statusBar = statusbar.New(m, statusbar.WithTitle(buildStatusBarTitle(m.appName, m.appVersion))) // Add the initial session to the supervisor sv.AddSession(ctx, initialApp, initialApp.Session(), initialWorkingDir, cleanup) diff --git a/pkg/tui/upgrade_hint.go b/pkg/tui/upgrade_hint.go new file mode 100644 index 000000000..4383bfca3 --- /dev/null +++ b/pkg/tui/upgrade_hint.go @@ -0,0 +1,20 @@ +package tui + +import ( + "github.com/docker/docker-agent/pkg/version/check" +) + +// buildStatusBarTitle returns the right-side string of the status bar: +// " ", optionally suffixed with "(update available: vX.Y.Z)" +// when a newer release tag has been observed in the local cache. +// +// Only cached results are consulted so the TUI never blocks on I/O at +// startup; the cache itself is refreshed asynchronously by the root +// PersistentPreRunE hook (see cmd/root/root.go). +func buildStatusBarTitle(appName, appVersion string) string { + base := appName + " " + appVersion + if res := check.LatestCached(appVersion); res.UpgradeAvailable() { + return base + " (update available: " + res.Latest + ")" + } + return base +} diff --git a/pkg/tui/upgrade_hint_test.go b/pkg/tui/upgrade_hint_test.go new file mode 100644 index 000000000..84bade7ae --- /dev/null +++ b/pkg/tui/upgrade_hint_test.go @@ -0,0 +1,71 @@ +package tui + +import ( + "encoding/json" + "os" + "path/filepath" + "strings" + "testing" + "time" + + "gotest.tools/v3/assert" + + "github.com/docker/docker-agent/pkg/paths" + "github.com/docker/docker-agent/pkg/version/check" +) + +// seedVersionCache writes a cache entry for the version checker into a per-test +// cache dir so that the check.LatestCached call inside buildStatusBarTitle +// returns deterministic results without ever hitting the network. +func seedVersionCache(t *testing.T, latest string) { + t.Helper() + + dir := t.TempDir() + prev := paths.GetCacheDir() + paths.SetCacheDir(dir) + t.Cleanup(func() { paths.SetCacheDir(prev) }) + + payload := struct { + LatestVersion string `json:"latest_version"` + CheckedAt int64 `json:"checked_at"` + }{ + LatestVersion: latest, + CheckedAt: time.Now().Unix(), + } + data, err := json.Marshal(payload) + assert.NilError(t, err) + + // Mirror the package's on-disk filename — kept intentionally simple + // rather than re-exporting the constant from check. + assert.NilError(t, os.WriteFile(filepath.Join(dir, "version-check.json"), data, 0o600)) +} + +func TestBuildStatusBarTitle_NoUpgrade(t *testing.T) { + seedVersionCache(t, "v1.0.0") + + got := buildStatusBarTitle("docker agent", "v1.0.0") + assert.Equal(t, "docker agent v1.0.0", got) +} + +func TestBuildStatusBarTitle_UpgradeAvailable(t *testing.T) { + seedVersionCache(t, "v1.2.3") + + got := buildStatusBarTitle("docker agent", "v1.0.0") + assert.Assert(t, strings.Contains(got, "docker agent v1.0.0")) + assert.Assert(t, strings.Contains(got, "update available: v1.2.3")) +} + +func TestBuildStatusBarTitle_DevBuildSilent(t *testing.T) { + seedVersionCache(t, "v1.2.3") + + got := buildStatusBarTitle("docker agent", "dev") + assert.Equal(t, "docker agent dev", got) +} + +func TestBuildStatusBarTitle_DisabledSilent(t *testing.T) { + seedVersionCache(t, "v1.2.3") + t.Setenv(check.DisableEnvVar, "1") + + got := buildStatusBarTitle("docker agent", "v1.0.0") + assert.Equal(t, "docker agent v1.0.0", got) +} diff --git a/pkg/version/check/check.go b/pkg/version/check/check.go new file mode 100644 index 000000000..d2109e8fa --- /dev/null +++ b/pkg/version/check/check.go @@ -0,0 +1,367 @@ +// Package check provides a best-effort upgrade check against GitHub releases. +// +// The check is intentionally lightweight: +// - The latest release tag is fetched from the GitHub REST API. +// - The result is cached on disk for 24h to avoid hitting the API on every +// invocation. +// - The check is opt-out via the DOCKER_AGENT_DISABLE_VERSION_CHECK=1 +// environment variable. +// - Failures (offline, rate-limited, parse errors, dev builds, …) are +// swallowed: the user simply does not see an upgrade hint. +// +// Callers retrieve the latest known version with [Latest]. To refresh the +// cache asynchronously without blocking startup, call [RefreshAsync]. +package check + +import ( + "context" + "encoding/json" + "errors" + "fmt" + "io" + "log/slog" + "net/http" + "os" + "path/filepath" + "strconv" + "strings" + "sync" + "time" + + "github.com/docker/docker-agent/pkg/paths" +) + +// DisableEnvVar is the environment variable that disables the version check +// when set to a truthy value (1, true, yes, on, …). It is honoured by both +// [Latest] and [RefreshAsync]. +const DisableEnvVar = "DOCKER_AGENT_DISABLE_VERSION_CHECK" + +// CacheTTL is how long a fetched release entry stays valid on disk before a +// refresh is attempted. +const CacheTTL = 24 * time.Hour + +// FetchTimeout bounds a single HTTP fetch to the GitHub API. +const FetchTimeout = 5 * time.Second + +// defaultReleasesURL is the GitHub REST endpoint used to fetch the latest +// stable release. Pre-release tags are excluded by `/releases/latest`. +const defaultReleasesURL = "https://api.github.com/repos/docker/docker-agent/releases/latest" + +// cacheFileName is the on-disk cache filename, stored under [paths.GetCacheDir]. +const cacheFileName = "version-check.json" + +// cacheEntry is the payload persisted under [paths.GetCacheDir]/[cacheFileName]. +type cacheEntry struct { + // LatestVersion is the latest release tag observed (e.g. "v1.53.0"). + LatestVersion string `json:"latest_version"` + // CheckedAt is the unix-second timestamp of the last successful fetch. + CheckedAt int64 `json:"checked_at"` +} + +// fresh reports whether the cache entry is still within [CacheTTL]. +func (e cacheEntry) fresh(now time.Time) bool { + if e.CheckedAt <= 0 { + return false + } + return now.Sub(time.Unix(e.CheckedAt, 0)) < CacheTTL +} + +// Result describes the outcome of an upgrade check. +type Result struct { + // Current is the running binary's version string. + Current string + // Latest is the latest known stable release tag, or "" if unknown. + Latest string +} + +// UpgradeAvailable returns true when [Result.Latest] is strictly newer than +// [Result.Current]. A "dev" build never reports an upgrade, since its version +// is not comparable to a release tag. +func (r Result) UpgradeAvailable() bool { + if r.Latest == "" || r.Current == "" || r.Current == "dev" { + return false + } + return IsNewer(r.Latest, r.Current) +} + +// Latest returns the most recently cached release, refreshing from the GitHub +// API only when the cache is stale (or missing) and the check is enabled. +// +// If the network call fails for any reason, the stale cache (if any) is +// returned so the caller can still surface a — possibly out of date — hint. +// If there is nothing usable to return, an empty string is returned alongside +// a nil error: the upgrade hint is best-effort and must not surface errors to +// the user. +func Latest(ctx context.Context, current string) Result { + res := Result{Current: current} + + if disabled() { + return res + } + + cache, _ := readCache() + + if cache.fresh(time.Now()) { + res.Latest = cache.LatestVersion + return res + } + + // Cache is stale or missing: try to refresh synchronously, with a tight + // timeout to avoid blocking the caller. + fetchCtx, cancel := context.WithTimeout(ctx, FetchTimeout) + defer cancel() + + tag, err := fetchLatestTag(fetchCtx, defaultReleasesURL) + if err != nil { + slog.Debug("Version check fetch failed", "error", err) + // Fall back to the (stale) cached value — better than nothing. + res.Latest = cache.LatestVersion + return res + } + + if err := writeCache(cacheEntry{LatestVersion: tag, CheckedAt: time.Now().Unix()}); err != nil { + slog.Debug("Version check cache write failed", "error", err) + } + + res.Latest = tag + return res +} + +// LatestCached returns the most recently cached release without ever issuing +// a network call. It is intended for callers (such as the TUI) that must not +// block on I/O at startup; combine with [RefreshAsync] to keep the cache warm. +func LatestCached(current string) Result { + res := Result{Current: current} + if disabled() { + return res + } + cache, _ := readCache() + res.Latest = cache.LatestVersion + return res +} + +// RefreshAsync triggers a background refresh of the version check cache when +// it is stale, returning immediately. Errors are logged at debug level and +// otherwise ignored. +// +// The returned channel is closed once the goroutine completes; tests use it +// to deterministically wait for completion. Production callers can ignore it. +func RefreshAsync(ctx context.Context) <-chan struct{} { + done := make(chan struct{}) + + if disabled() { + close(done) + return done + } + + cache, _ := readCache() + if cache.fresh(time.Now()) { + close(done) + return done + } + + go func() { + defer close(done) + + fetchCtx, cancel := context.WithTimeout(ctx, FetchTimeout) + defer cancel() + + tag, err := fetchLatestTag(fetchCtx, defaultReleasesURL) + if err != nil { + slog.Debug("Async version check fetch failed", "error", err) + return + } + if err := writeCache(cacheEntry{LatestVersion: tag, CheckedAt: time.Now().Unix()}); err != nil { + slog.Debug("Async version check cache write failed", "error", err) + } + }() + + return done +} + +// disabled reports whether the version check has been turned off via the +// [DisableEnvVar] environment variable. +func disabled() bool { + v := strings.ToLower(strings.TrimSpace(os.Getenv(DisableEnvVar))) + switch v { + case "1", "true", "yes", "on": + return true + } + return false +} + +// fetchLatestTag returns the `tag_name` field of the latest stable release. +// +// The endpoint is parameterised to keep the function unit-testable. +func fetchLatestTag(ctx context.Context, url string) (string, error) { + req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, http.NoBody) + if err != nil { + return "", err + } + // GitHub recommends an explicit Accept header for the v3 REST API. + req.Header.Set("Accept", "application/vnd.github+json") + req.Header.Set("X-GitHub-Api-Version", "2022-11-28") + + resp, err := http.DefaultClient.Do(req) + if err != nil { + return "", err + } + defer resp.Body.Close() + + if resp.StatusCode != http.StatusOK { + // Drain a little of the body for diagnostics but cap it so a + // misbehaving server can't blow up memory. + body, _ := io.ReadAll(io.LimitReader(resp.Body, 512)) + return "", fmt.Errorf("unexpected status %d: %s", resp.StatusCode, strings.TrimSpace(string(body))) + } + + var payload struct { + TagName string `json:"tag_name"` + } + if err := json.NewDecoder(io.LimitReader(resp.Body, 1<<20)).Decode(&payload); err != nil { + return "", fmt.Errorf("decode release payload: %w", err) + } + if payload.TagName == "" { + return "", errors.New("release payload missing tag_name") + } + return payload.TagName, nil +} + +// cacheMu guards reads and writes to the on-disk cache file. The cache is +// tiny so we serialise access rather than relying on filesystem atomicity. +var cacheMu sync.Mutex + +// cachePath returns the absolute path of the cache file. +func cachePath() string { + return filepath.Join(paths.GetCacheDir(), cacheFileName) +} + +// readCache returns the cached entry, or a zero entry if the file is missing +// or unreadable. Callers should treat a zero entry as "no cache". +func readCache() (cacheEntry, error) { + cacheMu.Lock() + defer cacheMu.Unlock() + + data, err := os.ReadFile(cachePath()) + if err != nil { + if errors.Is(err, os.ErrNotExist) { + return cacheEntry{}, nil + } + return cacheEntry{}, err + } + + var entry cacheEntry + if err := json.Unmarshal(data, &entry); err != nil { + return cacheEntry{}, err + } + return entry, nil +} + +// writeCache persists the given entry to disk, creating the cache directory +// if necessary. +func writeCache(entry cacheEntry) error { + cacheMu.Lock() + defer cacheMu.Unlock() + + dir := paths.GetCacheDir() + if err := os.MkdirAll(dir, 0o755); err != nil { + return fmt.Errorf("create cache dir: %w", err) + } + + data, err := json.Marshal(entry) + if err != nil { + return err + } + + // Write+rename for atomicity so a partial write never produces a + // truncated JSON file that future reads would treat as corrupt. + tmp, err := os.CreateTemp(dir, cacheFileName+".*") + if err != nil { + return err + } + tmpName := tmp.Name() + if _, err := tmp.Write(data); err != nil { + tmp.Close() + os.Remove(tmpName) + return err + } + if err := tmp.Close(); err != nil { + os.Remove(tmpName) + return err + } + return os.Rename(tmpName, cachePath()) +} + +// IsNewer reports whether the semver-like tag `latest` is strictly greater +// than `current`. The comparison is intentionally tolerant: +// +// - A leading "v" is stripped from both sides. +// - Pre-release suffixes (e.g. "-rc.1") are ignored for the numeric +// comparison and treated as older than their release counterpart. +// - Components that fail to parse as integers are treated as zero, so +// malformed inputs simply don't trigger a notification. +// - When either input is empty or equal to "dev", the function returns +// false: development builds never get an upgrade prompt. +func IsNewer(latest, current string) bool { + if latest == "" || current == "" || current == "dev" || latest == "dev" { + return false + } + + la, lpre := splitPreRelease(strings.TrimPrefix(latest, "v")) + cu, cpre := splitPreRelease(strings.TrimPrefix(current, "v")) + + if cmp := compareNumericParts(la, cu); cmp != 0 { + return cmp > 0 + } + + // Numeric parts are equal: a release (no pre-release) outranks a + // pre-release of the same version (1.2.3 > 1.2.3-rc.1). + switch { + case lpre == "" && cpre != "": + return true + case lpre != "" && cpre == "": + return false + default: + // Both have (or don't have) a pre-release: avoid lexical surprises + // and consider them equal — no upgrade prompt. + return false + } +} + +// splitPreRelease separates the numeric part of a semver-like string from +// any "-pre" suffix. For example "1.2.3-rc.1" → ("1.2.3", "rc.1"). +func splitPreRelease(v string) (string, string) { + // Drop build metadata first ("+meta"), it never affects ordering. + if i := strings.Index(v, "+"); i >= 0 { + v = v[:i] + } + if numeric, pre, ok := strings.Cut(v, "-"); ok { + return numeric, pre + } + return v, "" +} + +// compareNumericParts compares dotted numeric strings ("1.2.3") component by +// component, returning -1, 0 or +1. Missing trailing components are treated +// as zero so "1.2" == "1.2.0". +func compareNumericParts(a, b string) int { + ap := strings.Split(a, ".") + bp := strings.Split(b, ".") + n := max(len(ap), len(bp)) + for i := range n { + var ai, bi int + if i < len(ap) { + ai, _ = strconv.Atoi(ap[i]) + } + if i < len(bp) { + bi, _ = strconv.Atoi(bp[i]) + } + if ai != bi { + if ai > bi { + return 1 + } + return -1 + } + } + return 0 +} diff --git a/pkg/version/check/check_test.go b/pkg/version/check/check_test.go new file mode 100644 index 000000000..9a0b96adc --- /dev/null +++ b/pkg/version/check/check_test.go @@ -0,0 +1,251 @@ +package check + +import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "testing" + "time" + + "gotest.tools/v3/assert" + + "github.com/docker/docker-agent/pkg/paths" +) + +// withCacheDir routes the package's on-disk cache to a per-test temp directory +// so tests don't pollute the user's real cache and don't interfere with each +// other. +func withCacheDir(t *testing.T) string { + t.Helper() + dir := t.TempDir() + prev := paths.GetCacheDir() + paths.SetCacheDir(dir) + t.Cleanup(func() { paths.SetCacheDir(prev) }) + return dir +} + +func TestIsNewer_BasicSemverOrdering(t *testing.T) { + tests := []struct { + name string + latest string + current string + want bool + }{ + {"patch newer", "v1.2.4", "v1.2.3", true}, + {"minor newer", "v1.3.0", "v1.2.9", true}, + {"major newer", "v2.0.0", "v1.99.0", true}, + {"equal", "v1.2.3", "v1.2.3", false}, + {"older", "v1.2.2", "v1.2.3", false}, + {"prefix v optional", "1.2.4", "v1.2.3", true}, + {"missing components", "v1.3", "v1.2.9", true}, + {"release beats prerelease", "v1.2.3", "v1.2.3-rc.1", true}, + {"prerelease loses to release", "v1.2.3-rc.1", "v1.2.3", false}, + {"build metadata ignored", "v1.2.4+abcdef", "v1.2.3", true}, + {"both prerelease equal numeric", "v1.2.3-rc.2", "v1.2.3-rc.1", false}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + assert.Equal(t, tc.want, IsNewer(tc.latest, tc.current)) + }) + } +} + +func TestIsNewer_EmptyAndDevAreNeverNewer(t *testing.T) { + assert.Equal(t, false, IsNewer("", "v1.2.3")) + assert.Equal(t, false, IsNewer("v1.2.3", "")) + assert.Equal(t, false, IsNewer("v1.2.3", "dev")) + assert.Equal(t, false, IsNewer("dev", "v1.2.3")) +} + +func TestResult_UpgradeAvailable(t *testing.T) { + assert.Equal(t, true, Result{Current: "v1.0.0", Latest: "v1.0.1"}.UpgradeAvailable()) + assert.Equal(t, false, Result{Current: "v1.0.1", Latest: "v1.0.0"}.UpgradeAvailable()) + assert.Equal(t, false, Result{Current: "dev", Latest: "v1.0.0"}.UpgradeAvailable()) + assert.Equal(t, false, Result{Current: "v1.0.0", Latest: ""}.UpgradeAvailable()) +} + +func TestFetchLatestTag_Success(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + assert.Equal(t, "application/vnd.github+json", r.Header.Get("Accept")) + _ = json.NewEncoder(w).Encode(map[string]any{"tag_name": "v9.9.9"}) + })) + t.Cleanup(srv.Close) + + tag, err := fetchLatestTag(t.Context(), srv.URL) + assert.NilError(t, err) + assert.Equal(t, "v9.9.9", tag) +} + +func TestFetchLatestTag_HTTPError(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + http.Error(w, "rate limited", http.StatusForbidden) + })) + t.Cleanup(srv.Close) + + _, err := fetchLatestTag(t.Context(), srv.URL) + assert.ErrorContains(t, err, "unexpected status 403") +} + +func TestFetchLatestTag_MalformedPayload(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte("{not json")) + })) + t.Cleanup(srv.Close) + + _, err := fetchLatestTag(t.Context(), srv.URL) + assert.ErrorContains(t, err, "decode release payload") +} + +func TestFetchLatestTag_MissingTag(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte(`{"foo":"bar"}`)) + })) + t.Cleanup(srv.Close) + + _, err := fetchLatestTag(t.Context(), srv.URL) + assert.ErrorContains(t, err, "missing tag_name") +} + +func TestCacheRoundTrip(t *testing.T) { + dir := withCacheDir(t) + + want := cacheEntry{LatestVersion: "v1.2.3", CheckedAt: time.Now().Unix()} + assert.NilError(t, writeCache(want)) + + // Cache file ended up where we expect. + _, err := os.Stat(filepath.Join(dir, cacheFileName)) + assert.NilError(t, err) + + got, err := readCache() + assert.NilError(t, err) + assert.Equal(t, want.LatestVersion, got.LatestVersion) + assert.Equal(t, want.CheckedAt, got.CheckedAt) +} + +func TestReadCache_MissingReturnsZero(t *testing.T) { + withCacheDir(t) + + got, err := readCache() + assert.NilError(t, err) + assert.Equal(t, cacheEntry{}, got) +} + +func TestReadCache_CorruptReturnsError(t *testing.T) { + dir := withCacheDir(t) + assert.NilError(t, os.WriteFile(filepath.Join(dir, cacheFileName), []byte("not-json"), 0o600)) + + _, err := readCache() + assert.Assert(t, err != nil, "expected error when cache is corrupt") +} + +func TestCacheFreshness(t *testing.T) { + now := time.Now() + fresh := cacheEntry{CheckedAt: now.Add(-1 * time.Hour).Unix()} + stale := cacheEntry{CheckedAt: now.Add(-48 * time.Hour).Unix()} + zero := cacheEntry{} + + assert.Equal(t, true, fresh.fresh(now)) + assert.Equal(t, false, stale.fresh(now)) + assert.Equal(t, false, zero.fresh(now)) +} + +func TestLatestCached_Disabled(t *testing.T) { + withCacheDir(t) + // Seed a cache entry to prove the function ignores it when disabled. + assert.NilError(t, writeCache(cacheEntry{LatestVersion: "v9.9.9", CheckedAt: time.Now().Unix()})) + + t.Setenv(DisableEnvVar, "1") + res := LatestCached("v1.0.0") + assert.Equal(t, "", res.Latest) +} + +func TestLatestCached_FromDisk(t *testing.T) { + withCacheDir(t) + assert.NilError(t, writeCache(cacheEntry{LatestVersion: "v9.9.9", CheckedAt: time.Now().Unix()})) + + res := LatestCached("v1.0.0") + assert.Equal(t, "v9.9.9", res.Latest) + assert.Equal(t, true, res.UpgradeAvailable()) +} + +func TestLatestCached_NeverFetches(t *testing.T) { + withCacheDir(t) + + // No cache file exists. LatestCached must NOT block on I/O even if we + // give it a context that's already canceled. + ctx, cancel := context.WithCancel(t.Context()) + cancel() + + // Ensure the disabled path is not taken. + t.Setenv(DisableEnvVar, "") + + done := make(chan struct{}) + go func() { + _ = LatestCached("v1.0.0") + _ = ctx + close(done) + }() + + select { + case <-done: + case <-time.After(2 * time.Second): + t.Fatal("LatestCached blocked unexpectedly") + } +} + +func TestRefreshAsync_Disabled(t *testing.T) { + withCacheDir(t) + t.Setenv(DisableEnvVar, "true") + + done := RefreshAsync(t.Context()) + select { + case <-done: + case <-time.After(time.Second): + t.Fatal("RefreshAsync did not return promptly when disabled") + } + + // Cache must remain empty. + got, err := readCache() + assert.NilError(t, err) + assert.Equal(t, "", got.LatestVersion) +} + +func TestRefreshAsync_FreshCacheSkipsRefresh(t *testing.T) { + withCacheDir(t) + // Pre-seed a fresh entry so RefreshAsync should be a no-op. + want := cacheEntry{LatestVersion: "v1.2.3", CheckedAt: time.Now().Unix()} + assert.NilError(t, writeCache(want)) + + done := RefreshAsync(t.Context()) + select { + case <-done: + case <-time.After(time.Second): + t.Fatal("RefreshAsync did not return promptly with fresh cache") + } + + got, err := readCache() + assert.NilError(t, err) + assert.Equal(t, want.LatestVersion, got.LatestVersion) + assert.Equal(t, want.CheckedAt, got.CheckedAt) +} + +func TestDisabled_Truthy(t *testing.T) { + for _, val := range []string{"1", "true", "True", "YES", "on"} { + t.Run(val, func(t *testing.T) { + t.Setenv(DisableEnvVar, val) + assert.Equal(t, true, disabled()) + }) + } +} + +func TestDisabled_Falsy(t *testing.T) { + for _, val := range []string{"", "0", "false", "no", "off", "anything-else"} { + t.Run(val, func(t *testing.T) { + t.Setenv(DisableEnvVar, val) + assert.Equal(t, false, disabled()) + }) + } +} From 0b50eb4d591567d8a7597799c3a49d457de337fd Mon Sep 17 00:00:00 2001 From: David Gageot Date: Wed, 29 Apr 2026 09:16:36 +0200 Subject: [PATCH 2/8] only run version check on docker agent run --- cmd/root/root.go | 7 ------- cmd/root/run.go | 8 ++++++++ cmd/root/version.go | 8 +++++--- 3 files changed, 13 insertions(+), 10 deletions(-) diff --git a/cmd/root/root.go b/cmd/root/root.go index a24a55c66..c670080de 100644 --- a/cmd/root/root.go +++ b/cmd/root/root.go @@ -21,7 +21,6 @@ import ( "github.com/docker/docker-agent/pkg/paths" "github.com/docker/docker-agent/pkg/telemetry" "github.com/docker/docker-agent/pkg/version" - "github.com/docker/docker-agent/pkg/version/check" ) type rootFlags struct { @@ -95,12 +94,6 @@ We collect anonymous usage data to help improve docker agent. To disable: telemetry.SetGlobalTelemetryDebugMode(flags.debugMode) - // Kick off a best-effort, background check for a newer release. - // Results are cached on disk; the next invocation can surface them - // without blocking on a network call. Disabled by setting - // DOCKER_AGENT_DISABLE_VERSION_CHECK=1. - check.RefreshAsync(cmd.Context()) - if flags.enableOtel { if err := initOTelSDK(cmd.Context()); err != nil { slog.Warn("Failed to initialize OpenTelemetry SDK", "error", err) diff --git a/cmd/root/run.go b/cmd/root/run.go index 4832ee2d4..831817a0a 100644 --- a/cmd/root/run.go +++ b/cmd/root/run.go @@ -29,6 +29,7 @@ import ( "github.com/docker/docker-agent/pkg/tui" "github.com/docker/docker-agent/pkg/tui/styles" "github.com/docker/docker-agent/pkg/userconfig" + "github.com/docker/docker-agent/pkg/version/check" ) type runExecFlags struct { @@ -145,6 +146,13 @@ func (f *runExecFlags) runRunCommand(cmd *cobra.Command, args []string) (command }() } + // Kick off a best-effort, background check for a newer release. Results + // are cached on disk so the TUI status bar (and a future `version` call) + // can surface an upgrade hint without blocking on a network call. Only + // `run`/`exec` triggers this; other subcommands never reach out to + // GitHub. Disabled by setting DOCKER_AGENT_DISABLE_VERSION_CHECK=1. + check.RefreshAsync(ctx) + if f.sandbox { return runInSandbox(ctx, cmd, args, &f.runConfig, f.sandboxTemplate, f.sbx) } diff --git a/cmd/root/version.go b/cmd/root/version.go index 7143fbeb1..2d69c6fa7 100644 --- a/cmd/root/version.go +++ b/cmd/root/version.go @@ -35,9 +35,11 @@ func runVersionCommand(cmd *cobra.Command, args []string) { out.Printf("%s version %s\n", commandName, version.Version) out.Printf("Commit: %s\n", version.Commit) - // Best-effort upgrade hint. Failures (offline, rate-limited, …) are - // silently ignored — the user simply does not see the line. - if res := check.Latest(cmd.Context(), version.Version); res.UpgradeAvailable() { + // Best-effort upgrade hint based on the cached result of the last + // `docker agent run`. We never reach out to GitHub from this subcommand; + // if the cache is empty (e.g. `run` has never been used) the hint is + // simply not shown. + if res := check.LatestCached(version.Version); res.UpgradeAvailable() { out.Printf("\nA newer version is available: %s\nRelease notes: https://github.com/docker/docker-agent/releases/tag/%s\n", res.Latest, res.Latest) } From 4e11e1b89512867a39a8744205899fa3f20c348a Mon Sep 17 00:00:00 2001 From: David Gageot Date: Wed, 29 Apr 2026 09:17:53 +0200 Subject: [PATCH 3/8] document DOCKER_AGENT_DISABLE_VERSION_CHECK env var --- docs/configuration/overview/index.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/docs/configuration/overview/index.md b/docs/configuration/overview/index.md index ff718448d..d58d1c6c0 100644 --- a/docs/configuration/overview/index.md +++ b/docs/configuration/overview/index.md @@ -161,6 +161,12 @@ API keys and secrets are read from environment variables — never stored in con | `DOCKER_AGENT_AUTO_INSTALL` | Set to `false` to disable automatic tool installation | | `DOCKER_AGENT_TOOLS_DIR` | Override the base directory for installed tools (default: `~/.cagent/tools/`) | +**Update Notifications:** + +| Variable | Description | +| ----------------------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `DOCKER_AGENT_DISABLE_VERSION_CHECK` | Set to `1`/`true` to disable the once-a-day GitHub release check used to surface an upgrade hint in the TUI status bar and the `version` subcommand. Only `docker agent run` ever performs the check. | +
⚠️ Important
From a6ef679132aacce93f1d64604d20c7285adf7c2e Mon Sep 17 00:00:00 2001 From: David Gageot Date: Wed, 29 Apr 2026 09:18:18 +0200 Subject: [PATCH 4/8] disable version check in container image --- Dockerfile | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/Dockerfile b/Dockerfile index 71814c26b..bb8102961 100644 --- a/Dockerfile +++ b/Dockerfile @@ -75,6 +75,10 @@ RUN apk add --no-cache ca-certificates docker-cli && \ ARG TARGETOS TARGETARCH ENV DOCKER_MCP_IN_CONTAINER=1 ENV TERM=xterm-256color +# Disable the once-a-day GitHub release check inside the container image: +# images are tagged immutably and can't self-upgrade, so the hint would be +# misleading and waste a network round-trip on every `run`. +ENV DOCKER_AGENT_DISABLE_VERSION_CHECK=1 COPY --from=docker/mcp-gateway:v2 /docker-mcp /usr/local/lib/docker/cli-plugins/ COPY --from=builder-linux /binaries/docker-agent-$TARGETOS-$TARGETARCH /docker-agent USER docker-agent From 14a220049a1f275947432c94ef3c37f5850c4b50 Mon Sep 17 00:00:00 2001 From: David Gageot Date: Wed, 29 Apr 2026 10:28:20 +0200 Subject: [PATCH 5/8] simplify version check package MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Drop the unused synchronous Latest() function, replace the Result struct with a plain LatestCached(current) string that returns "" when no upgrade is known, and remove the redundant cache mutex and write+rename dance — readCache already tolerates partial reads. Also expose a SeedCacheForTest helper so external tests no longer reach into the JSON layout. --- cmd/root/version.go | 5 +- pkg/tui/upgrade_hint.go | 8 +- pkg/tui/upgrade_hint_test.go | 82 +++------ pkg/version/check/check.go | 288 ++++++++++---------------------- pkg/version/check/check_test.go | 257 +++++++++++----------------- pkg/version/check/testing.go | 29 ++++ 6 files changed, 242 insertions(+), 427 deletions(-) create mode 100644 pkg/version/check/testing.go diff --git a/cmd/root/version.go b/cmd/root/version.go index 2d69c6fa7..9c8bdc562 100644 --- a/cmd/root/version.go +++ b/cmd/root/version.go @@ -39,8 +39,7 @@ func runVersionCommand(cmd *cobra.Command, args []string) { // `docker agent run`. We never reach out to GitHub from this subcommand; // if the cache is empty (e.g. `run` has never been used) the hint is // simply not shown. - if res := check.LatestCached(version.Version); res.UpgradeAvailable() { - out.Printf("\nA newer version is available: %s\nRelease notes: https://github.com/docker/docker-agent/releases/tag/%s\n", - res.Latest, res.Latest) + if latest := check.LatestCached(version.Version); latest != "" { + out.Printf("\nA newer version is available: %s\nRelease notes: https://github.com/docker/docker-agent/releases/tag/%s\n", latest, latest) } } diff --git a/pkg/tui/upgrade_hint.go b/pkg/tui/upgrade_hint.go index 4383bfca3..5f5ea6ee2 100644 --- a/pkg/tui/upgrade_hint.go +++ b/pkg/tui/upgrade_hint.go @@ -9,12 +9,12 @@ import ( // when a newer release tag has been observed in the local cache. // // Only cached results are consulted so the TUI never blocks on I/O at -// startup; the cache itself is refreshed asynchronously by the root -// PersistentPreRunE hook (see cmd/root/root.go). +// startup; the cache is refreshed in the background by `docker agent run` +// (see cmd/root/run.go). func buildStatusBarTitle(appName, appVersion string) string { base := appName + " " + appVersion - if res := check.LatestCached(appVersion); res.UpgradeAvailable() { - return base + " (update available: " + res.Latest + ")" + if latest := check.LatestCached(appVersion); latest != "" { + return base + " (update available: " + latest + ")" } return base } diff --git a/pkg/tui/upgrade_hint_test.go b/pkg/tui/upgrade_hint_test.go index 84bade7ae..370c200b9 100644 --- a/pkg/tui/upgrade_hint_test.go +++ b/pkg/tui/upgrade_hint_test.go @@ -1,71 +1,35 @@ package tui import ( - "encoding/json" - "os" - "path/filepath" "strings" "testing" - "time" "gotest.tools/v3/assert" - "github.com/docker/docker-agent/pkg/paths" "github.com/docker/docker-agent/pkg/version/check" ) -// seedVersionCache writes a cache entry for the version checker into a per-test -// cache dir so that the check.LatestCached call inside buildStatusBarTitle -// returns deterministic results without ever hitting the network. -func seedVersionCache(t *testing.T, latest string) { - t.Helper() - - dir := t.TempDir() - prev := paths.GetCacheDir() - paths.SetCacheDir(dir) - t.Cleanup(func() { paths.SetCacheDir(prev) }) - - payload := struct { - LatestVersion string `json:"latest_version"` - CheckedAt int64 `json:"checked_at"` - }{ - LatestVersion: latest, - CheckedAt: time.Now().Unix(), - } - data, err := json.Marshal(payload) - assert.NilError(t, err) - - // Mirror the package's on-disk filename — kept intentionally simple - // rather than re-exporting the constant from check. - assert.NilError(t, os.WriteFile(filepath.Join(dir, "version-check.json"), data, 0o600)) -} - -func TestBuildStatusBarTitle_NoUpgrade(t *testing.T) { - seedVersionCache(t, "v1.0.0") - - got := buildStatusBarTitle("docker agent", "v1.0.0") - assert.Equal(t, "docker agent v1.0.0", got) -} - -func TestBuildStatusBarTitle_UpgradeAvailable(t *testing.T) { - seedVersionCache(t, "v1.2.3") - - got := buildStatusBarTitle("docker agent", "v1.0.0") - assert.Assert(t, strings.Contains(got, "docker agent v1.0.0")) - assert.Assert(t, strings.Contains(got, "update available: v1.2.3")) -} - -func TestBuildStatusBarTitle_DevBuildSilent(t *testing.T) { - seedVersionCache(t, "v1.2.3") - - got := buildStatusBarTitle("docker agent", "dev") - assert.Equal(t, "docker agent dev", got) -} - -func TestBuildStatusBarTitle_DisabledSilent(t *testing.T) { - seedVersionCache(t, "v1.2.3") - t.Setenv(check.DisableEnvVar, "1") - - got := buildStatusBarTitle("docker agent", "v1.0.0") - assert.Equal(t, "docker agent v1.0.0", got) +func TestBuildStatusBarTitle(t *testing.T) { + t.Run("no upgrade", func(t *testing.T) { + check.SeedCacheForTest(t, "v1.0.0") + assert.Equal(t, "docker agent v1.0.0", buildStatusBarTitle("docker agent", "v1.0.0")) + }) + + t.Run("upgrade available", func(t *testing.T) { + check.SeedCacheForTest(t, "v1.2.3") + got := buildStatusBarTitle("docker agent", "v1.0.0") + assert.Assert(t, strings.Contains(got, "docker agent v1.0.0")) + assert.Assert(t, strings.Contains(got, "update available: v1.2.3")) + }) + + t.Run("dev build is silent", func(t *testing.T) { + check.SeedCacheForTest(t, "v1.2.3") + assert.Equal(t, "docker agent dev", buildStatusBarTitle("docker agent", "dev")) + }) + + t.Run("disabled is silent", func(t *testing.T) { + check.SeedCacheForTest(t, "v1.2.3") + t.Setenv(check.DisableEnvVar, "1") + assert.Equal(t, "docker agent v1.0.0", buildStatusBarTitle("docker agent", "v1.0.0")) + }) } diff --git a/pkg/version/check/check.go b/pkg/version/check/check.go index d2109e8fa..dfde4cb46 100644 --- a/pkg/version/check/check.go +++ b/pkg/version/check/check.go @@ -1,16 +1,16 @@ -// Package check provides a best-effort upgrade check against GitHub releases. +// Package check provides a best-effort upgrade hint built from the GitHub +// releases of docker/docker-agent. // -// The check is intentionally lightweight: -// - The latest release tag is fetched from the GitHub REST API. -// - The result is cached on disk for 24h to avoid hitting the API on every -// invocation. -// - The check is opt-out via the DOCKER_AGENT_DISABLE_VERSION_CHECK=1 -// environment variable. -// - Failures (offline, rate-limited, parse errors, dev builds, …) are +// Behaviour: +// - The latest release tag is fetched in the background by [RefreshAsync], +// called once per `docker agent run` invocation. +// - The result is cached on disk for 24h so subsequent reads are instant. +// - [LatestCached] never touches the network: it only consults that cache, +// so callers (TUI status bar, `version` subcommand) can surface a hint +// without blocking on I/O. +// - The whole feature is opt-out via [DisableEnvVar]. +// - All errors (offline, rate-limited, parse errors, dev builds, …) are // swallowed: the user simply does not see an upgrade hint. -// -// Callers retrieve the latest known version with [Latest]. To refresh the -// cache asynchronously without blocking startup, call [RefreshAsync]. package check import ( @@ -25,127 +25,45 @@ import ( "path/filepath" "strconv" "strings" - "sync" "time" "github.com/docker/docker-agent/pkg/paths" ) // DisableEnvVar is the environment variable that disables the version check -// when set to a truthy value (1, true, yes, on, …). It is honoured by both -// [Latest] and [RefreshAsync]. +// when set to a truthy value (1, true, yes, on, …). const DisableEnvVar = "DOCKER_AGENT_DISABLE_VERSION_CHECK" -// CacheTTL is how long a fetched release entry stays valid on disk before a -// refresh is attempted. -const CacheTTL = 24 * time.Hour - -// FetchTimeout bounds a single HTTP fetch to the GitHub API. -const FetchTimeout = 5 * time.Second - -// defaultReleasesURL is the GitHub REST endpoint used to fetch the latest -// stable release. Pre-release tags are excluded by `/releases/latest`. -const defaultReleasesURL = "https://api.github.com/repos/docker/docker-agent/releases/latest" - -// cacheFileName is the on-disk cache filename, stored under [paths.GetCacheDir]. -const cacheFileName = "version-check.json" - -// cacheEntry is the payload persisted under [paths.GetCacheDir]/[cacheFileName]. -type cacheEntry struct { - // LatestVersion is the latest release tag observed (e.g. "v1.53.0"). - LatestVersion string `json:"latest_version"` - // CheckedAt is the unix-second timestamp of the last successful fetch. - CheckedAt int64 `json:"checked_at"` -} - -// fresh reports whether the cache entry is still within [CacheTTL]. -func (e cacheEntry) fresh(now time.Time) bool { - if e.CheckedAt <= 0 { - return false - } - return now.Sub(time.Unix(e.CheckedAt, 0)) < CacheTTL -} - -// Result describes the outcome of an upgrade check. -type Result struct { - // Current is the running binary's version string. - Current string - // Latest is the latest known stable release tag, or "" if unknown. - Latest string -} - -// UpgradeAvailable returns true when [Result.Latest] is strictly newer than -// [Result.Current]. A "dev" build never reports an upgrade, since its version -// is not comparable to a release tag. -func (r Result) UpgradeAvailable() bool { - if r.Latest == "" || r.Current == "" || r.Current == "dev" { - return false - } - return IsNewer(r.Latest, r.Current) -} +const ( + cacheTTL = 24 * time.Hour + fetchTimeout = 5 * time.Second + releasesURL = "https://api.github.com/repos/docker/docker-agent/releases/latest" + cacheFileName = "version-check.json" +) -// Latest returns the most recently cached release, refreshing from the GitHub -// API only when the cache is stale (or missing) and the check is enabled. +// LatestCached returns the latest known release tag if it is strictly newer +// than current, or "" otherwise. // -// If the network call fails for any reason, the stale cache (if any) is -// returned so the caller can still surface a — possibly out of date — hint. -// If there is nothing usable to return, an empty string is returned alongside -// a nil error: the upgrade hint is best-effort and must not surface errors to -// the user. -func Latest(ctx context.Context, current string) Result { - res := Result{Current: current} - - if disabled() { - return res - } - - cache, _ := readCache() - - if cache.fresh(time.Now()) { - res.Latest = cache.LatestVersion - return res - } - - // Cache is stale or missing: try to refresh synchronously, with a tight - // timeout to avoid blocking the caller. - fetchCtx, cancel := context.WithTimeout(ctx, FetchTimeout) - defer cancel() - - tag, err := fetchLatestTag(fetchCtx, defaultReleasesURL) - if err != nil { - slog.Debug("Version check fetch failed", "error", err) - // Fall back to the (stale) cached value — better than nothing. - res.Latest = cache.LatestVersion - return res - } - - if err := writeCache(cacheEntry{LatestVersion: tag, CheckedAt: time.Now().Unix()}); err != nil { - slog.Debug("Version check cache write failed", "error", err) - } - - res.Latest = tag - return res -} - -// LatestCached returns the most recently cached release without ever issuing -// a network call. It is intended for callers (such as the TUI) that must not -// block on I/O at startup; combine with [RefreshAsync] to keep the cache warm. -func LatestCached(current string) Result { - res := Result{Current: current} - if disabled() { - return res - } - cache, _ := readCache() - res.Latest = cache.LatestVersion - return res +// The function never reaches out to the network — it only consults the local +// cache populated by [RefreshAsync]. It also returns "" when the check is +// disabled or when current is "dev" (development build). +func LatestCached(current string) string { + if disabled() || current == "" || current == "dev" { + return "" + } + entry, _ := readCache() + if !IsNewer(entry.LatestVersion, current) { + return "" + } + return entry.LatestVersion } -// RefreshAsync triggers a background refresh of the version check cache when -// it is stale, returning immediately. Errors are logged at debug level and +// RefreshAsync triggers a background refresh of the on-disk cache when it is +// stale, returning immediately. Errors are logged at debug level and // otherwise ignored. // -// The returned channel is closed once the goroutine completes; tests use it -// to deterministically wait for completion. Production callers can ignore it. +// The returned channel is closed once the goroutine completes. Tests use it +// to deterministically wait for completion; production callers can ignore it. func RefreshAsync(ctx context.Context) <-chan struct{} { done := make(chan struct{}) @@ -153,9 +71,7 @@ func RefreshAsync(ctx context.Context) <-chan struct{} { close(done) return done } - - cache, _ := readCache() - if cache.fresh(time.Now()) { + if entry, _ := readCache(); entry.fresh(time.Now()) { close(done) return done } @@ -163,27 +79,26 @@ func RefreshAsync(ctx context.Context) <-chan struct{} { go func() { defer close(done) - fetchCtx, cancel := context.WithTimeout(ctx, FetchTimeout) + fetchCtx, cancel := context.WithTimeout(ctx, fetchTimeout) defer cancel() - tag, err := fetchLatestTag(fetchCtx, defaultReleasesURL) + tag, err := fetchLatestTag(fetchCtx, releasesURL) if err != nil { - slog.Debug("Async version check fetch failed", "error", err) + slog.Debug("Version check fetch failed", "error", err) return } - if err := writeCache(cacheEntry{LatestVersion: tag, CheckedAt: time.Now().Unix()}); err != nil { - slog.Debug("Async version check cache write failed", "error", err) + if err := writeCache(tag); err != nil { + slog.Debug("Version check cache write failed", "error", err) } }() return done } -// disabled reports whether the version check has been turned off via the -// [DisableEnvVar] environment variable. +// disabled reports whether the version check has been turned off via +// [DisableEnvVar]. func disabled() bool { - v := strings.ToLower(strings.TrimSpace(os.Getenv(DisableEnvVar))) - switch v { + switch strings.ToLower(strings.TrimSpace(os.Getenv(DisableEnvVar))) { case "1", "true", "yes", "on": return true } @@ -191,14 +106,12 @@ func disabled() bool { } // fetchLatestTag returns the `tag_name` field of the latest stable release. -// // The endpoint is parameterised to keep the function unit-testable. func fetchLatestTag(ctx context.Context, url string) (string, error) { req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, http.NoBody) if err != nil { return "", err } - // GitHub recommends an explicit Accept header for the v3 REST API. req.Header.Set("Accept", "application/vnd.github+json") req.Header.Set("X-GitHub-Api-Version", "2022-11-28") @@ -209,8 +122,6 @@ func fetchLatestTag(ctx context.Context, url string) (string, error) { defer resp.Body.Close() if resp.StatusCode != http.StatusOK { - // Drain a little of the body for diagnostics but cap it so a - // misbehaving server can't blow up memory. body, _ := io.ReadAll(io.LimitReader(resp.Body, 512)) return "", fmt.Errorf("unexpected status %d: %s", resp.StatusCode, strings.TrimSpace(string(body))) } @@ -227,9 +138,16 @@ func fetchLatestTag(ctx context.Context, url string) (string, error) { return payload.TagName, nil } -// cacheMu guards reads and writes to the on-disk cache file. The cache is -// tiny so we serialise access rather than relying on filesystem atomicity. -var cacheMu sync.Mutex +// cacheEntry is the JSON payload persisted to disk. +type cacheEntry struct { + LatestVersion string `json:"latest_version"` + CheckedAt int64 `json:"checked_at"` +} + +// fresh reports whether the entry is still within [cacheTTL]. +func (e cacheEntry) fresh(now time.Time) bool { + return e.CheckedAt > 0 && now.Sub(time.Unix(e.CheckedAt, 0)) < cacheTTL +} // cachePath returns the absolute path of the cache file. func cachePath() string { @@ -237,11 +155,9 @@ func cachePath() string { } // readCache returns the cached entry, or a zero entry if the file is missing -// or unreadable. Callers should treat a zero entry as "no cache". +// or unreadable. The file is small enough that we do not worry about partial +// reads: if Unmarshal fails, callers simply see "no cache" for one call. func readCache() (cacheEntry, error) { - cacheMu.Lock() - defer cacheMu.Unlock() - data, err := os.ReadFile(cachePath()) if err != nil { if errors.Is(err, os.ErrNotExist) { @@ -249,7 +165,6 @@ func readCache() (cacheEntry, error) { } return cacheEntry{}, err } - var entry cacheEntry if err := json.Unmarshal(data, &entry); err != nil { return cacheEntry{}, err @@ -257,98 +172,63 @@ func readCache() (cacheEntry, error) { return entry, nil } -// writeCache persists the given entry to disk, creating the cache directory -// if necessary. -func writeCache(entry cacheEntry) error { - cacheMu.Lock() - defer cacheMu.Unlock() - - dir := paths.GetCacheDir() - if err := os.MkdirAll(dir, 0o755); err != nil { +// writeCache persists the given release tag along with the current timestamp. +func writeCache(latest string) error { + if err := os.MkdirAll(paths.GetCacheDir(), 0o755); err != nil { return fmt.Errorf("create cache dir: %w", err) } - - data, err := json.Marshal(entry) - if err != nil { - return err - } - - // Write+rename for atomicity so a partial write never produces a - // truncated JSON file that future reads would treat as corrupt. - tmp, err := os.CreateTemp(dir, cacheFileName+".*") + data, err := json.Marshal(cacheEntry{LatestVersion: latest, CheckedAt: time.Now().Unix()}) if err != nil { return err } - tmpName := tmp.Name() - if _, err := tmp.Write(data); err != nil { - tmp.Close() - os.Remove(tmpName) - return err - } - if err := tmp.Close(); err != nil { - os.Remove(tmpName) - return err - } - return os.Rename(tmpName, cachePath()) + return os.WriteFile(cachePath(), data, 0o600) } -// IsNewer reports whether the semver-like tag `latest` is strictly greater -// than `current`. The comparison is intentionally tolerant: +// IsNewer reports whether the semver-like tag latest is strictly greater than +// current. The comparison is intentionally tolerant: // // - A leading "v" is stripped from both sides. -// - Pre-release suffixes (e.g. "-rc.1") are ignored for the numeric -// comparison and treated as older than their release counterpart. -// - Components that fail to parse as integers are treated as zero, so -// malformed inputs simply don't trigger a notification. -// - When either input is empty or equal to "dev", the function returns -// false: development builds never get an upgrade prompt. +// - Build metadata ("+meta") is ignored. +// - A pre-release ("-rc.1") sorts strictly older than the same release. +// - Components that fail to parse as integers are treated as 0, so +// malformed inputs simply do not trigger a notification. +// - Empty strings or "dev" never compare as newer. func IsNewer(latest, current string) bool { if latest == "" || current == "" || current == "dev" || latest == "dev" { return false } - la, lpre := splitPreRelease(strings.TrimPrefix(latest, "v")) - cu, cpre := splitPreRelease(strings.TrimPrefix(current, "v")) + la, lpre := splitVersion(latest) + cu, cpre := splitVersion(current) - if cmp := compareNumericParts(la, cu); cmp != 0 { + if cmp := compareNumeric(la, cu); cmp != 0 { return cmp > 0 } - - // Numeric parts are equal: a release (no pre-release) outranks a - // pre-release of the same version (1.2.3 > 1.2.3-rc.1). - switch { - case lpre == "" && cpre != "": - return true - case lpre != "" && cpre == "": - return false - default: - // Both have (or don't have) a pre-release: avoid lexical surprises - // and consider them equal — no upgrade prompt. - return false - } + // Equal numeric parts: a release outranks a pre-release of the same + // version (1.2.3 > 1.2.3-rc.1). Otherwise treat as equal. + return lpre == "" && cpre != "" } -// splitPreRelease separates the numeric part of a semver-like string from -// any "-pre" suffix. For example "1.2.3-rc.1" → ("1.2.3", "rc.1"). -func splitPreRelease(v string) (string, string) { - // Drop build metadata first ("+meta"), it never affects ordering. +// splitVersion strips a leading "v", drops "+build" metadata, and splits off +// any "-prerelease" suffix. For example "v1.2.3-rc.1+meta" → ("1.2.3", "rc.1"). +func splitVersion(v string) (numeric, pre string) { + v = strings.TrimPrefix(v, "v") if i := strings.Index(v, "+"); i >= 0 { v = v[:i] } - if numeric, pre, ok := strings.Cut(v, "-"); ok { - return numeric, pre + if num, p, ok := strings.Cut(v, "-"); ok { + return num, p } return v, "" } -// compareNumericParts compares dotted numeric strings ("1.2.3") component by +// compareNumeric compares dotted numeric strings ("1.2.3") component by // component, returning -1, 0 or +1. Missing trailing components are treated // as zero so "1.2" == "1.2.0". -func compareNumericParts(a, b string) int { +func compareNumeric(a, b string) int { ap := strings.Split(a, ".") bp := strings.Split(b, ".") - n := max(len(ap), len(bp)) - for i := range n { + for i := range max(len(ap), len(bp)) { var ai, bi int if i < len(ap) { ai, _ = strconv.Atoi(ap[i]) diff --git a/pkg/version/check/check_test.go b/pkg/version/check/check_test.go index 9a0b96adc..a3508fbd7 100644 --- a/pkg/version/check/check_test.go +++ b/pkg/version/check/check_test.go @@ -1,7 +1,6 @@ package check import ( - "context" "encoding/json" "net/http" "net/http/httptest" @@ -11,23 +10,9 @@ import ( "time" "gotest.tools/v3/assert" - - "github.com/docker/docker-agent/pkg/paths" ) -// withCacheDir routes the package's on-disk cache to a per-test temp directory -// so tests don't pollute the user's real cache and don't interfere with each -// other. -func withCacheDir(t *testing.T) string { - t.Helper() - dir := t.TempDir() - prev := paths.GetCacheDir() - paths.SetCacheDir(dir) - t.Cleanup(func() { paths.SetCacheDir(prev) }) - return dir -} - -func TestIsNewer_BasicSemverOrdering(t *testing.T) { +func TestIsNewer(t *testing.T) { tests := []struct { name string latest string @@ -45,6 +30,10 @@ func TestIsNewer_BasicSemverOrdering(t *testing.T) { {"prerelease loses to release", "v1.2.3-rc.1", "v1.2.3", false}, {"build metadata ignored", "v1.2.4+abcdef", "v1.2.3", true}, {"both prerelease equal numeric", "v1.2.3-rc.2", "v1.2.3-rc.1", false}, + {"empty latest", "", "v1.2.3", false}, + {"empty current", "v1.2.3", "", false}, + {"dev current never upgrades", "v1.2.3", "dev", false}, + {"dev latest never upgrades", "dev", "v1.2.3", false}, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { @@ -53,199 +42,153 @@ func TestIsNewer_BasicSemverOrdering(t *testing.T) { } } -func TestIsNewer_EmptyAndDevAreNeverNewer(t *testing.T) { - assert.Equal(t, false, IsNewer("", "v1.2.3")) - assert.Equal(t, false, IsNewer("v1.2.3", "")) - assert.Equal(t, false, IsNewer("v1.2.3", "dev")) - assert.Equal(t, false, IsNewer("dev", "v1.2.3")) -} - -func TestResult_UpgradeAvailable(t *testing.T) { - assert.Equal(t, true, Result{Current: "v1.0.0", Latest: "v1.0.1"}.UpgradeAvailable()) - assert.Equal(t, false, Result{Current: "v1.0.1", Latest: "v1.0.0"}.UpgradeAvailable()) - assert.Equal(t, false, Result{Current: "dev", Latest: "v1.0.0"}.UpgradeAvailable()) - assert.Equal(t, false, Result{Current: "v1.0.0", Latest: ""}.UpgradeAvailable()) -} - -func TestFetchLatestTag_Success(t *testing.T) { - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - assert.Equal(t, "application/vnd.github+json", r.Header.Get("Accept")) - _ = json.NewEncoder(w).Encode(map[string]any{"tag_name": "v9.9.9"}) - })) - t.Cleanup(srv.Close) - - tag, err := fetchLatestTag(t.Context(), srv.URL) - assert.NilError(t, err) - assert.Equal(t, "v9.9.9", tag) -} - -func TestFetchLatestTag_HTTPError(t *testing.T) { - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { - http.Error(w, "rate limited", http.StatusForbidden) - })) - t.Cleanup(srv.Close) - - _, err := fetchLatestTag(t.Context(), srv.URL) - assert.ErrorContains(t, err, "unexpected status 403") -} - -func TestFetchLatestTag_MalformedPayload(t *testing.T) { - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { - _, _ = w.Write([]byte("{not json")) - })) - t.Cleanup(srv.Close) - - _, err := fetchLatestTag(t.Context(), srv.URL) - assert.ErrorContains(t, err, "decode release payload") -} - -func TestFetchLatestTag_MissingTag(t *testing.T) { - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { - _, _ = w.Write([]byte(`{"foo":"bar"}`)) - })) - t.Cleanup(srv.Close) - - _, err := fetchLatestTag(t.Context(), srv.URL) - assert.ErrorContains(t, err, "missing tag_name") +func TestFetchLatestTag(t *testing.T) { + tests := []struct { + name string + handler http.HandlerFunc + wantTag string + wantErrSub string + }{ + { + name: "success", + handler: func(w http.ResponseWriter, r *http.Request) { + assert.Equal(t, "application/vnd.github+json", r.Header.Get("Accept")) + _ = json.NewEncoder(w).Encode(map[string]any{"tag_name": "v9.9.9"}) + }, + wantTag: "v9.9.9", + }, + { + name: "http error", + handler: func(w http.ResponseWriter, _ *http.Request) { http.Error(w, "rate limited", http.StatusForbidden) }, + wantErrSub: "unexpected status 403", + }, + { + name: "malformed payload", + handler: func(w http.ResponseWriter, _ *http.Request) { _, _ = w.Write([]byte("{not json")) }, + wantErrSub: "decode release payload", + }, + { + name: "missing tag", + handler: func(w http.ResponseWriter, _ *http.Request) { _, _ = w.Write([]byte(`{"foo":"bar"}`)) }, + wantErrSub: "missing tag_name", + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + srv := httptest.NewServer(tc.handler) + t.Cleanup(srv.Close) + + tag, err := fetchLatestTag(t.Context(), srv.URL) + if tc.wantErrSub != "" { + assert.ErrorContains(t, err, tc.wantErrSub) + return + } + assert.NilError(t, err) + assert.Equal(t, tc.wantTag, tag) + }) + } } func TestCacheRoundTrip(t *testing.T) { - dir := withCacheDir(t) + SeedCacheForTest(t, "") - want := cacheEntry{LatestVersion: "v1.2.3", CheckedAt: time.Now().Unix()} - assert.NilError(t, writeCache(want)) + assert.NilError(t, writeCache("v1.2.3")) // Cache file ended up where we expect. - _, err := os.Stat(filepath.Join(dir, cacheFileName)) + _, err := os.Stat(cachePath()) assert.NilError(t, err) got, err := readCache() assert.NilError(t, err) - assert.Equal(t, want.LatestVersion, got.LatestVersion) - assert.Equal(t, want.CheckedAt, got.CheckedAt) + assert.Equal(t, "v1.2.3", got.LatestVersion) + assert.Assert(t, got.fresh(time.Now())) } func TestReadCache_MissingReturnsZero(t *testing.T) { - withCacheDir(t) + SeedCacheForTest(t, "") got, err := readCache() assert.NilError(t, err) assert.Equal(t, cacheEntry{}, got) } -func TestReadCache_CorruptReturnsError(t *testing.T) { - dir := withCacheDir(t) - assert.NilError(t, os.WriteFile(filepath.Join(dir, cacheFileName), []byte("not-json"), 0o600)) +func TestReadCache_CorruptReturnsZero(t *testing.T) { + SeedCacheForTest(t, "") + assert.NilError(t, os.WriteFile(filepath.Join(filepath.Dir(cachePath()), cacheFileName), []byte("not-json"), 0o600)) - _, err := readCache() - assert.Assert(t, err != nil, "expected error when cache is corrupt") + got, _ := readCache() + assert.Equal(t, cacheEntry{}, got) } func TestCacheFreshness(t *testing.T) { now := time.Now() - fresh := cacheEntry{CheckedAt: now.Add(-1 * time.Hour).Unix()} - stale := cacheEntry{CheckedAt: now.Add(-48 * time.Hour).Unix()} - zero := cacheEntry{} - - assert.Equal(t, true, fresh.fresh(now)) - assert.Equal(t, false, stale.fresh(now)) - assert.Equal(t, false, zero.fresh(now)) -} - -func TestLatestCached_Disabled(t *testing.T) { - withCacheDir(t) - // Seed a cache entry to prove the function ignores it when disabled. - assert.NilError(t, writeCache(cacheEntry{LatestVersion: "v9.9.9", CheckedAt: time.Now().Unix()})) - - t.Setenv(DisableEnvVar, "1") - res := LatestCached("v1.0.0") - assert.Equal(t, "", res.Latest) -} - -func TestLatestCached_FromDisk(t *testing.T) { - withCacheDir(t) - assert.NilError(t, writeCache(cacheEntry{LatestVersion: "v9.9.9", CheckedAt: time.Now().Unix()})) - - res := LatestCached("v1.0.0") - assert.Equal(t, "v9.9.9", res.Latest) - assert.Equal(t, true, res.UpgradeAvailable()) + assert.Assert(t, cacheEntry{CheckedAt: now.Add(-1 * time.Hour).Unix()}.fresh(now), "1h-old entry should be fresh") + assert.Assert(t, !cacheEntry{CheckedAt: now.Add(-48 * time.Hour).Unix()}.fresh(now), "48h-old entry should be stale") + assert.Assert(t, !cacheEntry{}.fresh(now), "zero entry should be stale") } -func TestLatestCached_NeverFetches(t *testing.T) { - withCacheDir(t) +func TestLatestCached(t *testing.T) { + t.Run("empty cache returns empty", func(t *testing.T) { + SeedCacheForTest(t, "") + assert.Equal(t, "", LatestCached("v1.0.0")) + }) - // No cache file exists. LatestCached must NOT block on I/O even if we - // give it a context that's already canceled. - ctx, cancel := context.WithCancel(t.Context()) - cancel() + t.Run("cache newer than current returns latest", func(t *testing.T) { + SeedCacheForTest(t, "v9.9.9") + assert.Equal(t, "v9.9.9", LatestCached("v1.0.0")) + }) - // Ensure the disabled path is not taken. - t.Setenv(DisableEnvVar, "") + t.Run("cache older than current returns empty", func(t *testing.T) { + SeedCacheForTest(t, "v1.0.0") + assert.Equal(t, "", LatestCached("v9.9.9")) + }) - done := make(chan struct{}) - go func() { - _ = LatestCached("v1.0.0") - _ = ctx - close(done) - }() + t.Run("dev current never reports upgrade", func(t *testing.T) { + SeedCacheForTest(t, "v9.9.9") + assert.Equal(t, "", LatestCached("dev")) + }) - select { - case <-done: - case <-time.After(2 * time.Second): - t.Fatal("LatestCached blocked unexpectedly") - } + t.Run("disabled returns empty even when cache has upgrade", func(t *testing.T) { + SeedCacheForTest(t, "v9.9.9") + t.Setenv(DisableEnvVar, "1") + assert.Equal(t, "", LatestCached("v1.0.0")) + }) } func TestRefreshAsync_Disabled(t *testing.T) { - withCacheDir(t) + SeedCacheForTest(t, "") t.Setenv(DisableEnvVar, "true") - done := RefreshAsync(t.Context()) - select { - case <-done: - case <-time.After(time.Second): - t.Fatal("RefreshAsync did not return promptly when disabled") - } + <-RefreshAsync(t.Context()) - // Cache must remain empty. got, err := readCache() assert.NilError(t, err) assert.Equal(t, "", got.LatestVersion) } func TestRefreshAsync_FreshCacheSkipsRefresh(t *testing.T) { - withCacheDir(t) - // Pre-seed a fresh entry so RefreshAsync should be a no-op. - want := cacheEntry{LatestVersion: "v1.2.3", CheckedAt: time.Now().Unix()} - assert.NilError(t, writeCache(want)) - - done := RefreshAsync(t.Context()) - select { - case <-done: - case <-time.After(time.Second): - t.Fatal("RefreshAsync did not return promptly with fresh cache") - } + SeedCacheForTest(t, "v1.2.3") + before, err := readCache() + assert.NilError(t, err) - got, err := readCache() + <-RefreshAsync(t.Context()) + + after, err := readCache() assert.NilError(t, err) - assert.Equal(t, want.LatestVersion, got.LatestVersion) - assert.Equal(t, want.CheckedAt, got.CheckedAt) + assert.Equal(t, before, after, "fresh cache must not be touched") } -func TestDisabled_Truthy(t *testing.T) { +func TestDisabled(t *testing.T) { for _, val := range []string{"1", "true", "True", "YES", "on"} { - t.Run(val, func(t *testing.T) { + t.Run("truthy/"+val, func(t *testing.T) { t.Setenv(DisableEnvVar, val) - assert.Equal(t, true, disabled()) + assert.Assert(t, disabled()) }) } -} - -func TestDisabled_Falsy(t *testing.T) { for _, val := range []string{"", "0", "false", "no", "off", "anything-else"} { - t.Run(val, func(t *testing.T) { + t.Run("falsy/"+val, func(t *testing.T) { t.Setenv(DisableEnvVar, val) - assert.Equal(t, false, disabled()) + assert.Assert(t, !disabled()) }) } } diff --git a/pkg/version/check/testing.go b/pkg/version/check/testing.go new file mode 100644 index 000000000..7fe0f9b97 --- /dev/null +++ b/pkg/version/check/testing.go @@ -0,0 +1,29 @@ +package check + +import ( + "testing" + + "github.com/docker/docker-agent/pkg/paths" +) + +// SeedCacheForTest points the cache directory at a per-test temp dir and +// pre-populates it with the given release tag (or leaves it empty if latest +// is ""). It is intended for unit tests in other packages that want to +// observe [LatestCached] returning a deterministic value without hitting the +// network. +// +// The cache directory override is restored on test cleanup. +func SeedCacheForTest(tb testing.TB, latest string) { + tb.Helper() + + prev := paths.GetCacheDir() + paths.SetCacheDir(tb.TempDir()) + tb.Cleanup(func() { paths.SetCacheDir(prev) }) + + if latest == "" { + return + } + if err := writeCache(latest); err != nil { + tb.Fatalf("seed version cache: %v", err) + } +} From f6492dfdf88ff399662b3b268d370e9e37b32931 Mon Sep 17 00:00:00 2001 From: David Gageot Date: Wed, 29 Apr 2026 10:42:11 +0200 Subject: [PATCH 6/8] Add HTTP client timeout and redirect limit to version check - Use custom http.Client with 5s timeout instead of DefaultClient - Limit redirects to 3 to prevent SSRF and redirect loops - Add test for redirect limit enforcement - Add test for concurrent RefreshAsync calls to verify thread safety --- pkg/version/check/check.go | 28 +++++++++++++------------- pkg/version/check/check_test.go | 35 +++++++++++++++++++++++++++++++++ 2 files changed, 49 insertions(+), 14 deletions(-) diff --git a/pkg/version/check/check.go b/pkg/version/check/check.go index dfde4cb46..0308972e2 100644 --- a/pkg/version/check/check.go +++ b/pkg/version/check/check.go @@ -1,16 +1,3 @@ -// Package check provides a best-effort upgrade hint built from the GitHub -// releases of docker/docker-agent. -// -// Behaviour: -// - The latest release tag is fetched in the background by [RefreshAsync], -// called once per `docker agent run` invocation. -// - The result is cached on disk for 24h so subsequent reads are instant. -// - [LatestCached] never touches the network: it only consults that cache, -// so callers (TUI status bar, `version` subcommand) can surface a hint -// without blocking on I/O. -// - The whole feature is opt-out via [DisableEnvVar]. -// - All errors (offline, rate-limited, parse errors, dev builds, …) are -// swallowed: the user simply does not see an upgrade hint. package check import ( @@ -108,6 +95,19 @@ func disabled() bool { // fetchLatestTag returns the `tag_name` field of the latest stable release. // The endpoint is parameterised to keep the function unit-testable. func fetchLatestTag(ctx context.Context, url string) (string, error) { + // Use a custom client with timeout and redirect limit to prevent + // SSRF/redirect loops. The context timeout in RefreshAsync is a backstop; + // this client-level timeout ensures the request doesn't hang. + client := &http.Client{ + Timeout: fetchTimeout, + CheckRedirect: func(req *http.Request, via []*http.Request) error { + if len(via) >= 3 { + return fmt.Errorf("stopped after 3 redirects") + } + return nil + }, + } + req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, http.NoBody) if err != nil { return "", err @@ -115,7 +115,7 @@ func fetchLatestTag(ctx context.Context, url string) (string, error) { req.Header.Set("Accept", "application/vnd.github+json") req.Header.Set("X-GitHub-Api-Version", "2022-11-28") - resp, err := http.DefaultClient.Do(req) + resp, err := client.Do(req) if err != nil { return "", err } diff --git a/pkg/version/check/check_test.go b/pkg/version/check/check_test.go index a3508fbd7..d2fdb3911 100644 --- a/pkg/version/check/check_test.go +++ b/pkg/version/check/check_test.go @@ -6,6 +6,7 @@ import ( "net/http/httptest" "os" "path/filepath" + "sync" "testing" "time" @@ -192,3 +193,37 @@ func TestDisabled(t *testing.T) { }) } } + +func TestFetchLatestTag_RedirectLimit(t *testing.T) { + redirectCount := 0 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + redirectCount++ + if redirectCount <= 5 { + http.Redirect(w, r, "/redirect", http.StatusFound) + return + } + _ = json.NewEncoder(w).Encode(map[string]any{"tag_name": "v1.0.0"}) + })) + t.Cleanup(srv.Close) + + _, err := fetchLatestTag(t.Context(), srv.URL) + assert.ErrorContains(t, err, "stopped after 3 redirects") +} + +func TestRefreshAsync_ConcurrentCalls(t *testing.T) { + SeedCacheForTest(t, "") + + const numCalls = 10 + var wg sync.WaitGroup + wg.Add(numCalls) + + for i := 0; i < numCalls; i++ { + go func() { + defer wg.Done() + <-RefreshAsync(t.Context()) + }() + } + + wg.Wait() + // If we get here without panicking or racing, the test passes +} From 2764d172ca2bf2237703db562a4eaf1eeb6096c1 Mon Sep 17 00:00:00 2001 From: David Gageot Date: Wed, 29 Apr 2026 10:43:41 +0200 Subject: [PATCH 7/8] Fix linter issues in version check - Use errors.New instead of fmt.Errorf for static strings - Use integer range loop (Go 1.22+) - Fix formatting --- pkg/version/check/check.go | 2 +- pkg/version/check/check_test.go | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/pkg/version/check/check.go b/pkg/version/check/check.go index 0308972e2..2d47a2ecf 100644 --- a/pkg/version/check/check.go +++ b/pkg/version/check/check.go @@ -102,7 +102,7 @@ func fetchLatestTag(ctx context.Context, url string) (string, error) { Timeout: fetchTimeout, CheckRedirect: func(req *http.Request, via []*http.Request) error { if len(via) >= 3 { - return fmt.Errorf("stopped after 3 redirects") + return errors.New("stopped after 3 redirects") } return nil }, diff --git a/pkg/version/check/check_test.go b/pkg/version/check/check_test.go index d2fdb3911..0e053babe 100644 --- a/pkg/version/check/check_test.go +++ b/pkg/version/check/check_test.go @@ -216,14 +216,14 @@ func TestRefreshAsync_ConcurrentCalls(t *testing.T) { const numCalls = 10 var wg sync.WaitGroup wg.Add(numCalls) - - for i := 0; i < numCalls; i++ { + + for range numCalls { go func() { defer wg.Done() <-RefreshAsync(t.Context()) }() } - + wg.Wait() // If we get here without panicking or racing, the test passes } From cd379d4a36aaada1ed7aec0b19ab756b7a03e847 Mon Sep 17 00:00:00 2001 From: David Gageot Date: Wed, 29 Apr 2026 10:44:58 +0200 Subject: [PATCH 8/8] Use atomic write for version check cache Write to a temporary file and rename atomically to prevent cache corruption when multiple docker agent processes run concurrently. --- pkg/version/check/check.go | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/pkg/version/check/check.go b/pkg/version/check/check.go index 2d47a2ecf..148f88912 100644 --- a/pkg/version/check/check.go +++ b/pkg/version/check/check.go @@ -173,6 +173,7 @@ func readCache() (cacheEntry, error) { } // writeCache persists the given release tag along with the current timestamp. +// It uses atomic write-and-rename to prevent corruption from concurrent writes. func writeCache(latest string) error { if err := os.MkdirAll(paths.GetCacheDir(), 0o755); err != nil { return fmt.Errorf("create cache dir: %w", err) @@ -181,7 +182,14 @@ func writeCache(latest string) error { if err != nil { return err } - return os.WriteFile(cachePath(), data, 0o600) + + // Write to a temporary file first, then atomically rename it to prevent + // corruption if multiple processes write concurrently. + tmpPath := cachePath() + ".tmp" + if err := os.WriteFile(tmpPath, data, 0o600); err != nil { + return err + } + return os.Rename(tmpPath, cachePath()) } // IsNewer reports whether the semver-like tag latest is strictly greater than