diff --git a/internal/boxcli/root.go b/internal/boxcli/root.go index c8bda151779..cfe5082f29e 100644 --- a/internal/boxcli/root.go +++ b/internal/boxcli/root.go @@ -17,6 +17,7 @@ import ( "go.jetify.com/devbox/internal/cmdutil" "go.jetify.com/devbox/internal/debug" "go.jetify.com/devbox/internal/launcher" + "go.jetify.com/devbox/internal/plugin" "go.jetify.com/devbox/internal/telemetry" "go.jetify.com/devbox/internal/vercheck" ) @@ -101,6 +102,8 @@ func RootCmd() *cobra.Command { func Execute(ctx context.Context, args []string) int { defer debug.Recover() + // Git plugins keep one clone per repo for the whole command. + defer plugin.RemoveGitClones() rootCmd := RootCmd() exe := midcobra.New(rootCmd) // Runs first because it might re-run the command with another devbox diff --git a/internal/plugin/cache.go b/internal/plugin/cache.go new file mode 100644 index 00000000000..b332bb8ba0d --- /dev/null +++ b/internal/plugin/cache.go @@ -0,0 +1,83 @@ +// Copyright 2024 Jetify Inc. and contributors. All rights reserved. +// Use of this source code is governed by the license in the LICENSE file. + +package plugin + +import ( + "errors" + "fmt" + "log/slog" + "os" + "time" + + "go.jetify.com/pkg/filecache" +) + +// defaultRemoteCacheTTL is how long content fetched from a remote plugin at a +// branch, tag, or default branch is used before it's fetched again. +const defaultRemoteCacheTTL = 24 * time.Hour + +// pinnedCacheTTL is used for content fetched at a full commit hash, which +// can't change. filecache needs an expiration, so use one far in the future. +const pinnedCacheTTL = 100 * 365 * 24 * time.Hour + +// remoteCacheTTL returns how long to cache content fetched from a remote +// plugin at the given rev (which may be empty). +func remoteCacheTTL(rev string) (time.Duration, error) { + if isFullCommitHash(rev) { + return pinnedCacheTTL, nil + } + + // This is a stopgap until plugin is stored in lockfile. + // DEVBOX_X indicates this is an experimental env var. + // Use DEVBOX_X_GITHUB_PLUGIN_CACHE_TTL to override the default TTL. + // e.g. DEVBOX_X_GITHUB_PLUGIN_CACHE_TTL=1h will cache the plugin for 1 hour. + // Note: If you want to disable cache, we recommend using a low second value instead of zero to + // ensure only one network request is made. + ttlStr := os.Getenv("DEVBOX_X_GITHUB_PLUGIN_CACHE_TTL") + if ttlStr == "" { + return defaultRemoteCacheTTL, nil + } + ttl, err := time.ParseDuration(ttlStr) + if err != nil { + return 0, fmt.Errorf("invalid DEVBOX_X_GITHUB_PLUGIN_CACHE_TTL=%q: %w", ttlStr, err) + } + return ttl, nil +} + +// getOrFetch returns the cached value for key if it hasn't expired. Otherwise +// it calls fetch and caches the result for ttl. If fetch fails and an expired +// value is cached, it returns the expired value so that remote plugins keep +// working when the network or the remote is unavailable. +func getOrFetch( + cache *filecache.Cache[[]byte], + key string, + ttl time.Duration, + fetch func() ([]byte, error), +) ([]byte, error) { + cached, cacheErr := cache.Get(key) + if cacheErr == nil { + return cached, nil + } + if !filecache.IsCacheMiss(cacheErr) { + return nil, cacheErr + } + + content, err := fetch() + if err != nil { + if errors.Is(cacheErr, filecache.Expired) { + slog.Warn("failed to fetch remote plugin content, using expired cached copy", + "key", key, "err", err) + return cached, nil + } + return nil, err + } + return content, cache.Set(key, content, ttl) +} + +// isFullCommitHash reports whether s is a full 40-character commit hash. +// Short hashes, branches, and tags are not, since what they point to can +// change. +func isFullCommitHash(s string) bool { + return s != "" && !isBranchName(s) +} diff --git a/internal/plugin/cache_test.go b/internal/plugin/cache_test.go new file mode 100644 index 00000000000..f5552909e5d --- /dev/null +++ b/internal/plugin/cache_test.go @@ -0,0 +1,363 @@ +// Copyright 2024 Jetify Inc. and contributors. All rights reserved. +// Use of this source code is governed by the license in the LICENSE file. + +package plugin + +import ( + "errors" + "fmt" + "net/http" + "net/http/httptest" + "os" + "path" + "path/filepath" + "sync/atomic" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.jetify.com/devbox/nix/flake" + "go.jetify.com/pkg/filecache" +) + +const testCommit = "a1b2c3d4e5f6789012345678901234567890abcd" + +// useTempPluginCaches points the remote plugin caches at a temp dir and +// removes any git clones made by the test. +func useTempPluginCaches(t *testing.T) { + t.Helper() + dir := t.TempDir() + oldGit, oldGitHub := gitCache, githubCache + gitCache = filecache.New("devbox/plugin/git", filecache.WithCacheDir[[]byte](dir)) + githubCache = filecache.New("devbox/plugin/github", filecache.WithCacheDir[[]byte](dir)) + RemoveGitClones() + t.Cleanup(func() { + gitCache, githubCache = oldGit, oldGitHub + RemoveGitClones() + }) +} + +func TestGetOrFetch(t *testing.T) { + useTempPluginCaches(t) + cache := gitCache + errOffline := errors.New("offline") + fetchOK := func(calls *int) func() ([]byte, error) { + return func() ([]byte, error) { + *calls++ + return []byte("new"), nil + } + } + fetchFail := func(calls *int) func() ([]byte, error) { + return func() ([]byte, error) { + *calls++ + return nil, errOffline + } + } + + t.Run("fresh entry is used without fetching", func(t *testing.T) { + require.NoError(t, cache.Set("fresh", []byte("old"), time.Hour)) + calls := 0 + got, err := getOrFetch(cache, "fresh", time.Hour, fetchFail(&calls)) + require.NoError(t, err) + assert.Equal(t, "old", string(got)) + assert.Equal(t, 0, calls) + }) + + t.Run("expired entry is refreshed", func(t *testing.T) { + require.NoError(t, cache.SetWithTime("refresh", []byte("old"), time.Now().Add(-time.Hour))) + calls := 0 + got, err := getOrFetch(cache, "refresh", time.Hour, fetchOK(&calls)) + require.NoError(t, err) + assert.Equal(t, "new", string(got)) + assert.Equal(t, 1, calls) + + // The refreshed value is cached. + got, err = getOrFetch(cache, "refresh", time.Hour, fetchFail(&calls)) + require.NoError(t, err) + assert.Equal(t, "new", string(got)) + assert.Equal(t, 1, calls) + }) + + t.Run("expired entry is used when fetch fails", func(t *testing.T) { + require.NoError(t, cache.SetWithTime("stale", []byte("old"), time.Now().Add(-time.Hour))) + calls := 0 + got, err := getOrFetch(cache, "stale", time.Hour, fetchFail(&calls)) + require.NoError(t, err) + assert.Equal(t, "old", string(got)) + assert.Equal(t, 1, calls) + }) + + t.Run("fetch error is returned when nothing is cached", func(t *testing.T) { + calls := 0 + _, err := getOrFetch(cache, "missing", time.Hour, fetchFail(&calls)) + require.ErrorIs(t, err, errOffline) + assert.Equal(t, 1, calls) + }) +} + +func TestRemoteCacheTTL(t *testing.T) { + t.Setenv("DEVBOX_X_GITHUB_PLUGIN_CACHE_TTL", "") + for rev, want := range map[string]time.Duration{ + "": defaultRemoteCacheTTL, + "abc1234": defaultRemoteCacheTTL, // short hash + "main": defaultRemoteCacheTTL, + testCommit: pinnedCacheTTL, + "A1B2C3D4E5F6789012345678901234567890ABCD": pinnedCacheTTL, + } { + got, err := remoteCacheTTL(rev) + require.NoError(t, err) + assert.Equal(t, want, got, "rev %q", rev) + } + + // The env var only overrides the TTL for content that isn't pinned to a + // commit. + t.Setenv("DEVBOX_X_GITHUB_PLUGIN_CACHE_TTL", "1h") + got, err := remoteCacheTTL("main") + require.NoError(t, err) + assert.Equal(t, time.Hour, got) + got, err = remoteCacheTTL(testCommit) + require.NoError(t, err) + assert.Equal(t, pinnedCacheTTL, got) +} + +// fakeGitRemote stubs out git clones. Each clone writes files into a new +// temp dir. +type fakeGitRemote struct { + files map[string]string + clones int + err error +} + +func stubGitClone(t *testing.T, files map[string]string) *fakeGitRemote { + t.Helper() + remote := &fakeGitRemote{files: files} + old := cloneGitRepo + cloneGitRepo = func(*gitPlugin) (string, error) { + remote.clones++ + if remote.err != nil { + return "", remote.err + } + dir := t.TempDir() + for name, content := range remote.files { + p := filepath.Join(dir, name) + if err := os.MkdirAll(filepath.Dir(p), 0o755); err != nil { + return "", err + } + if err := os.WriteFile(p, []byte(content), 0o644); err != nil { + return "", err + } + } + return dir, nil + } + t.Cleanup(func() { cloneGitRepo = old }) + return remote +} + +func newTestGitPlugin(ref, rev, dir string) *gitPlugin { + return &gitPlugin{ + ref: &flake.Ref{ + Type: flake.TypeGit, + URL: "https://example.com/org/plugins.git", + Ref: ref, + Rev: rev, + Dir: dir, + }, + name: "test", + } +} + +func readGitFile(t *testing.T, p *gitPlugin, subpath string) string { + t.Helper() + content, err := p.FileContent(subpath) + require.NoError(t, err) + return string(content) +} + +func TestGitPluginClonesOncePerRepoRef(t *testing.T) { + useTempPluginCaches(t) + remote := stubGitClone(t, map[string]string{ + "plugin.json": "root plugin", + "a.txt": "a", + "sub/plugin.json": "sub plugin", + "sub/b.txt": "b", + }) + + root := newTestGitPlugin("main", "", "") + assert.Equal(t, "root plugin", readGitFile(t, root, "plugin.json")) + assert.Equal(t, "a", readGitFile(t, root, "a.txt")) + + // Another plugin in the same repo and ref reuses the clone. + sub := newTestGitPlugin("main", "", "sub") + assert.Equal(t, "sub plugin", readGitFile(t, sub, "plugin.json")) + assert.Equal(t, "b", readGitFile(t, sub, "b.txt")) + assert.Equal(t, 1, remote.clones) + + // A different ref needs its own clone. + other := newTestGitPlugin("dev", "", "") + assert.Equal(t, "a", readGitFile(t, other, "a.txt")) + assert.Equal(t, 2, remote.clones) +} + +func TestGitPluginCommitPinnedDoesNotExpire(t *testing.T) { + useTempPluginCaches(t) + t.Setenv("DEVBOX_X_GITHUB_PLUGIN_CACHE_TTL", "1ns") + remote := stubGitClone(t, map[string]string{"plugin.json": "v1"}) + + p := newTestGitPlugin("main", testCommit, "") + assert.Equal(t, "v1", readGitFile(t, p, "plugin.json")) + + // Even with a new process (no clones) and a 1ns TTL override, the + // pinned content comes from the cache without cloning again. + RemoveGitClones() + remote.files["plugin.json"] = "v2" + assert.Equal(t, "v1", readGitFile(t, p, "plugin.json")) + assert.Equal(t, 1, remote.clones) +} + +func TestGitPluginUnpinnedExpires(t *testing.T) { + for name, rev := range map[string]string{ + "branch": "", + "short hash": "a1b2c3d", + } { + t.Run(name, func(t *testing.T) { + useTempPluginCaches(t) + t.Setenv("DEVBOX_X_GITHUB_PLUGIN_CACHE_TTL", "1ns") + remote := stubGitClone(t, map[string]string{"plugin.json": "v1"}) + + p := newTestGitPlugin("main", rev, "") + assert.Equal(t, "v1", readGitFile(t, p, "plugin.json")) + + RemoveGitClones() + remote.files["plugin.json"] = "v2" + assert.Equal(t, "v2", readGitFile(t, p, "plugin.json")) + assert.Equal(t, 2, remote.clones) + }) + } +} + +func TestGitPluginExpiredFallsBackWhenCloneFails(t *testing.T) { + useTempPluginCaches(t) + t.Setenv("DEVBOX_X_GITHUB_PLUGIN_CACHE_TTL", "1ns") + remote := stubGitClone(t, map[string]string{"plugin.json": "v1"}) + + p := newTestGitPlugin("main", "", "") + assert.Equal(t, "v1", readGitFile(t, p, "plugin.json")) + + RemoveGitClones() + remote.err = errors.New("network is unreachable") + assert.Equal(t, "v1", readGitFile(t, p, "plugin.json")) + assert.Equal(t, 2, remote.clones) +} + +func TestGitPluginCloneFailsWithNothingCached(t *testing.T) { + useTempPluginCaches(t) + remote := stubGitClone(t, nil) + remote.err = errors.New("network is unreachable") + + p := newTestGitPlugin("main", "", "") + _, err := p.FileContent("plugin.json") + require.ErrorIs(t, err, remote.err) + + // The failed clone isn't retried for other files in the same process. + _, err = p.FileContent("a.txt") + require.ErrorIs(t, err, remote.err) + assert.Equal(t, 1, remote.clones) +} + +// fakeGitHub serves plugin files over HTTP. Each response body is the +// requested file name and the current version. +type fakeGitHub struct { + server *httptest.Server + requests atomic.Int32 + version atomic.Int32 + status atomic.Int32 +} + +func stubGitHub(t *testing.T) *fakeGitHub { + t.Helper() + fake := &fakeGitHub{} + fake.version.Store(1) + fake.status.Store(http.StatusOK) + fake.server = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + fake.requests.Add(1) + w.WriteHeader(int(fake.status.Load())) + fmt.Fprintf(w, "%s v%d", path.Base(r.URL.Path), fake.version.Load()) + })) + t.Cleanup(fake.server.Close) + + old := githubContentBaseURL + githubContentBaseURL = fake.server.URL + "/" + t.Cleanup(func() { githubContentBaseURL = old }) + return fake +} + +func newTestGithubPlugin(ref, rev string) *githubPlugin { + return &githubPlugin{ + ref: flake.Ref{ + Type: flake.TypeGitHub, + Owner: "org", + Repo: "plugins", + Ref: ref, + Rev: rev, + Dir: "mongodb", + }, + name: "test", + } +} + +func readGithubPluginJSON(t *testing.T, plugin *githubPlugin) string { + t.Helper() + content, err := plugin.FileContent("plugin.json") + require.NoError(t, err) + return string(content) +} + +func TestGithubPluginHTTPClientHasTimeout(t *testing.T) { + assert.Positive(t, githubHTTPClient.Timeout) +} + +func TestGithubPluginCommitPinnedDoesNotExpire(t *testing.T) { + useTempPluginCaches(t) + t.Setenv("DEVBOX_X_GITHUB_PLUGIN_CACHE_TTL", "1ns") + fake := stubGitHub(t) + + plugin := newTestGithubPlugin("", testCommit) + assert.Equal(t, "plugin.json v1", readGithubPluginJSON(t, plugin)) + fake.version.Store(2) + assert.Equal(t, "plugin.json v1", readGithubPluginJSON(t, plugin)) + assert.Equal(t, int32(1), fake.requests.Load()) +} + +func TestGithubPluginBranchExpires(t *testing.T) { + useTempPluginCaches(t) + t.Setenv("DEVBOX_X_GITHUB_PLUGIN_CACHE_TTL", "1ns") + fake := stubGitHub(t) + + plugin := newTestGithubPlugin("main", "") + assert.Equal(t, "plugin.json v1", readGithubPluginJSON(t, plugin)) + fake.version.Store(2) + assert.Equal(t, "plugin.json v2", readGithubPluginJSON(t, plugin)) + assert.Equal(t, int32(2), fake.requests.Load()) +} + +func TestGithubPluginExpiredFallsBackWhenFetchFails(t *testing.T) { + useTempPluginCaches(t) + t.Setenv("DEVBOX_X_GITHUB_PLUGIN_CACHE_TTL", "1ns") + fake := stubGitHub(t) + + plugin := newTestGithubPlugin("main", "") + assert.Equal(t, "plugin.json v1", readGithubPluginJSON(t, plugin)) + + // Server error. + fake.status.Store(http.StatusInternalServerError) + assert.Equal(t, "plugin.json v1", readGithubPluginJSON(t, plugin)) + + // Network error. + fake.server.Close() + assert.Equal(t, "plugin.json v1", readGithubPluginJSON(t, plugin)) + + // Nothing cached for this file, so the error is returned. + _, err := plugin.FileContent("other.txt") + require.Error(t, err) +} diff --git a/internal/plugin/git.go b/internal/plugin/git.go index 939b2818d58..e44a191c6ec 100644 --- a/internal/plugin/git.go +++ b/internal/plugin/git.go @@ -9,7 +9,7 @@ import ( "os/exec" "path/filepath" "strings" - "time" + "sync" "go.jetify.com/devbox/nix/flake" "go.jetify.com/pkg/filecache" @@ -90,12 +90,56 @@ func (p *gitPlugin) Fetch() ([]byte, error) { return content, nil } -func (p *gitPlugin) cloneAndRead(subpath string) ([]byte, error) { +// gitClones holds one clone per repo/ref/rev for the life of the process, so +// that reading several files from a git plugin (plugin.json plus the files it +// creates) clones the repo once instead of once per file. A failed clone is +// remembered too, so an unreachable remote is only tried once per process. +var gitClones = struct { + sync.Mutex + clones map[string]gitClone +}{clones: map[string]gitClone{}} + +type gitClone struct { + dir string + err error +} + +// cloneGitRepo clones the plugin's repo into a new temporary directory and +// returns its path. Tests override it. +var cloneGitRepo = (*gitPlugin).clone + +// RemoveGitClones deletes the git plugin clones made by this process. +func RemoveGitClones() { + gitClones.Lock() + defer gitClones.Unlock() + for key, c := range gitClones.clones { + if c.dir != "" { + _ = os.RemoveAll(c.dir) + } + delete(gitClones.clones, key) + } +} + +// cloneDir returns a clone of the plugin's repo at its ref/rev, cloning it if +// this process hasn't already. Plugins that differ only by dir share a clone. +func (p *gitPlugin) cloneDir() (string, error) { + key := strings.Join([]string{p.getBaseURL(), p.ref.Ref, p.ref.Rev}, "\x00") + + gitClones.Lock() + defer gitClones.Unlock() + c, ok := gitClones.clones[key] + if !ok { + c.dir, c.err = cloneGitRepo(p) + gitClones.clones[key] = c + } + return c.dir, c.err +} + +func (p *gitPlugin) clone() (string, error) { tempDir, err := os.MkdirTemp("", "devbox-git-plugin-*") if err != nil { - return nil, fmt.Errorf("failed to create temp directory: %w", err) + return "", fmt.Errorf("failed to create temp directory: %w", err) } - defer os.RemoveAll(tempDir) baseURL := p.getBaseURL() @@ -121,7 +165,8 @@ func (p *gitPlugin) cloneAndRead(subpath string) ([]byte, error) { output, err := cloneCmd.CombinedOutput() if err != nil { - return nil, fmt.Errorf("failed to clone repository %s: %w\nOutput: %s", p.ref.URL, err, string(output)) + _ = os.RemoveAll(tempDir) + return "", fmt.Errorf("failed to clone repository %s: %w\nOutput: %s", p.ref.URL, err, string(output)) } if p.ref.Rev != "" { @@ -129,21 +174,27 @@ func (p *gitPlugin) cloneAndRead(subpath string) ([]byte, error) { checkoutCmd.Dir = tempDir output, err := checkoutCmd.CombinedOutput() if err != nil { - return nil, fmt.Errorf("failed to checkout revision %s: %w\nOutput: %s", p.ref.Rev, err, string(output)) + _ = os.RemoveAll(tempDir) + return "", fmt.Errorf("failed to checkout revision %s: %w\nOutput: %s", p.ref.Rev, err, string(output)) } } + return tempDir, nil +} - // Read file from repository root or specified directory - filePath := filepath.Join(tempDir, subpath) - if p.ref.Dir != "" { - filePath = filepath.Join(tempDir, p.ref.Dir, subpath) +// readFromClone reads subpath from the plugin's directory in a clone of its +// repo. +func (p *gitPlugin) readFromClone(subpath string) ([]byte, error) { + cloneDir, err := p.cloneDir() + if err != nil { + return nil, err } + // Read file from repository root or specified directory + filePath := filepath.Join(cloneDir, p.ref.Dir, subpath) content, err := os.ReadFile(filePath) if err != nil { return nil, fmt.Errorf("failed to read file %s: %w", filePath, err) } - return content, nil } @@ -194,22 +245,15 @@ func (p *gitPlugin) Hash() string { } func (p *gitPlugin) FileContent(subpath string) ([]byte, error) { - ttl := 24 * time.Hour - var err error - ttlStr := os.Getenv("DEVBOX_X_GITHUB_PLUGIN_CACHE_TTL") - if ttlStr != "" { - ttl, err = time.ParseDuration(ttlStr) - if err != nil { - return nil, fmt.Errorf("invalid DEVBOX_X_GITHUB_PLUGIN_CACHE_TTL=%q: %w", ttlStr, err) - } + // Content at a full commit hash is cached indefinitely. Anything else is + // cached for 24 hours. + ttl, err := remoteCacheTTL(p.ref.Rev) + if err != nil { + return nil, err } cacheKey := p.LockfileKey() + "/" + subpath + "/" + ttl.String() - return gitCache.GetOrSet(cacheKey, func() ([]byte, time.Duration, error) { - content, err := p.cloneAndRead(subpath) - if err != nil { - return nil, 0, err - } - return content, ttl, nil + return getOrFetch(gitCache, cacheKey, ttl, func() ([]byte, error) { + return p.readFromClone(subpath) }) } diff --git a/internal/plugin/git_test.go b/internal/plugin/git_test.go index 869525c13a7..b575b790878 100644 --- a/internal/plugin/git_test.go +++ b/internal/plugin/git_test.go @@ -450,11 +450,7 @@ func setupLocalGitRepo(t *testing.T, content string) string { } func TestGitPluginFileContentCache(t *testing.T) { - // Clear the git cache before and after the test to avoid pollution. - if err := gitCache.Clear(); err != nil { - t.Fatal(err) - } - t.Cleanup(func() { _ = gitCache.Clear() }) + useTempPluginCaches(t) repoURL := setupLocalGitRepo(t, `{"name": "test-plugin"}`) @@ -476,12 +472,14 @@ func TestGitPluginFileContentCache(t *testing.T) { t.Fatalf("unexpected content: %s", content1) } - // Delete the source repo. If the cache is working, FileContent should - // still return the cached value without attempting a clone. + // Delete the source repo and this process's clone. If the cache is + // working, FileContent should still return the cached value without + // attempting a clone. repoPath := repoURL[len("file://"):] if err := os.RemoveAll(repoPath); err != nil { t.Fatalf("failed to remove repo: %v", err) } + RemoveGitClones() content2, err := plugin.FileContent("plugin.json") if err != nil { @@ -492,11 +490,10 @@ func TestGitPluginFileContentCache(t *testing.T) { } } -func TestGitPluginFileContentCacheRespectsEnvVar(t *testing.T) { - if err := gitCache.Clear(); err != nil { - t.Fatal(err) - } - t.Cleanup(func() { _ = gitCache.Clear() }) +// TestGitPluginFileContentExpiredCloneFails checks, with a real git clone, +// that expired content is still used when the repo can't be cloned anymore. +func TestGitPluginFileContentExpiredCloneFails(t *testing.T) { + useTempPluginCaches(t) repoURL := setupLocalGitRepo(t, `{"name": "ttl-test"}`) @@ -520,22 +517,26 @@ func TestGitPluginFileContentCacheRespectsEnvVar(t *testing.T) { t.Fatalf("unexpected content: %s", content) } - // With a 1ns TTL the cache entry should already be expired. - // Delete the source repo — if the expired cache is not served, - // this will attempt a fresh clone and fail, proving the TTL works. + // With a 1ns TTL the cache entry is already expired. Delete the source + // repo and this process's clone so the refetch fails; the expired + // content should be returned instead of an error. repoPath := repoURL[len("file://"):] if err := os.RemoveAll(repoPath); err != nil { t.Fatalf("failed to remove repo: %v", err) } - _, err = plugin.FileContent("plugin.json") - if err == nil { - t.Fatal("expected error after cache expiry with deleted repo, but got nil") + RemoveGitClones() + content, err = plugin.FileContent("plugin.json") + if err != nil { + t.Fatalf("expected expired cached content, got error: %v", err) + } + if string(content) != `{"name": "ttl-test"}` { + t.Fatalf("unexpected content: %s", content) } } func TestGitPluginFileContentCacheInvalidTTL(t *testing.T) { + useTempPluginCaches(t) t.Setenv("DEVBOX_X_GITHUB_PLUGIN_CACHE_TTL", "not-a-duration") - t.Cleanup(func() { _ = gitCache.Clear() }) plugin := &gitPlugin{ ref: &flake.Ref{ @@ -599,9 +600,10 @@ func TestGitPluginCloneRefAndOldRev(t *testing.T) { Rev: oldRev, }, } - content, err := plugin.cloneAndRead("plugin.json") + t.Cleanup(RemoveGitClones) + content, err := plugin.readFromClone("plugin.json") if err != nil { - t.Fatalf("cloneAndRead failed: %v", err) + t.Fatalf("readFromClone failed: %v", err) } if string(content) != `{"name": "old"}` { t.Fatalf("unexpected content: %s", content) diff --git a/internal/plugin/github.go b/internal/plugin/github.go index a587b7642b1..5a09ac4af8e 100644 --- a/internal/plugin/github.go +++ b/internal/plugin/github.go @@ -22,6 +22,14 @@ import ( var githubCache = filecache.New[[]byte]("devbox/plugin/github") +// githubHTTPClient fetches plugin files from GitHub. The timeout keeps an +// unreachable or hanging network from blocking devbox indefinitely. +var githubHTTPClient = &http.Client{Timeout: 15 * time.Second} + +// githubContentBaseURL is where plugin files are downloaded from. Tests +// override it. +var githubContentBaseURL = "https://raw.githubusercontent.com/" + type githubPlugin struct { ref flake.Ref name string @@ -71,37 +79,28 @@ func (p *githubPlugin) FileContent(subpath string) ([]byte, error) { return nil, err } - // Cache for 24 hours. Once we store the plugin in the lockfile, we + // Content at a full commit hash is cached indefinitely. Anything else is + // cached for 24 hours. Once we store the plugin in the lockfile, we // should cache this indefinitely and only invalidate if the plugin // is updated. - ttl := 24 * time.Hour - - // This is a stopgap until plugin is stored in lockfile. - // DEVBOX_X indicates this is an experimental env var. - // Use DEVBOX_X_GITHUB_PLUGIN_CACHE_TTL to override the default TTL. - // e.g. DEVBOX_X_GITHUB_PLUGIN_CACHE_TTL=1h will cache the plugin for 1 hour. - // Note: If you want to disable cache, we recommend using a low second value instead of zero to - // ensure only one network request is made. - ttlStr := os.Getenv("DEVBOX_X_GITHUB_PLUGIN_CACHE_TTL") - if ttlStr != "" { - ttl, err = time.ParseDuration(ttlStr) - if err != nil { - return nil, err - } + ttl, err := remoteCacheTTL(p.ref.Rev) + if err != nil { + return nil, err } - return githubCache.GetOrSet( + return getOrFetch( + githubCache, contentURL+ttl.String(), - func() ([]byte, time.Duration, error) { + ttl, + func() ([]byte, error) { req, err := p.request(contentURL) if err != nil { - return nil, 0, err + return nil, err } - client := &http.Client{} - res, err := client.Do(req) + res, err := githubHTTPClient.Do(req) if err != nil { - return nil, 0, err + return nil, err } defer res.Body.Close() if res.StatusCode != http.StatusOK { @@ -112,7 +111,7 @@ func (p *githubPlugin) FileContent(subpath string) ([]byte, error) { getRedactedAuthHeader(req), ) } - return nil, 0, usererr.New( + return nil, usererr.New( "failed to get plugin %s @ %s (Status code %d).\n%s\nPlease make "+ "sure a plugin.json file exists in plugin directory.", p.LockfileKey(), @@ -121,12 +120,7 @@ func (p *githubPlugin) FileContent(subpath string) ([]byte, error) { authInfo, ) } - body, err := io.ReadAll(res.Body) - if err != nil { - return nil, 0, err - } - - return body, ttl, nil + return io.ReadAll(res.Body) }, ) } @@ -135,7 +129,7 @@ func (p *githubPlugin) url(subpath string) (string, error) { // Github redirects "master" to "main" in new repos. They don't do the reverse // so setting master here is better. return url.JoinPath( - "https://raw.githubusercontent.com/", + githubContentBaseURL, p.ref.Owner, p.ref.Repo, cmp.Or(p.ref.Rev, p.ref.Ref, "master"), diff --git a/specs/perf-improvements.md b/specs/perf-improvements.md index b55a796d56c..13b49fbfc53 100644 --- a/specs/perf-improvements.md +++ b/specs/perf-improvements.md @@ -188,7 +188,7 @@ out to `nix profile list --json`. Fix: read `/manifest.json` directly and parse it with the existing structs. Fall back to the command for unknown manifest versions. -### 11. Remote plugins: timeout, offline fallback, better caching +### 11. Remote plugins: timeout, offline fallback, better caching (done in #3029) **Win:** removes a once-a-day blocking network call. **Effort:** ~60–100 LOC.