diff --git a/agent/agent.go b/agent/agent.go index ffdacfb64ba75..cbb12f9655f5a 100644 --- a/agent/agent.go +++ b/agent/agent.go @@ -1176,12 +1176,6 @@ func (a *agent) handleManifest(manifestOK *checkpoint) func(ctx context.Context, } a.metrics.startupScriptSeconds.WithLabelValues(label).Set(dur) a.scriptRunner.StartCron() - if containerAPI := a.containerAPI.Load(); containerAPI != nil { - // Inform the container API that the agent is ready. - // This allows us to start watching for changes to - // the devcontainer configuration files. - containerAPI.SignalReady() - } }) if err != nil { return xerrors.Errorf("track conn goroutine: %w", err) diff --git a/agent/agentcontainers/api.go b/agent/agentcontainers/api.go index f2164c9a874ff..7fd42175db7d4 100644 --- a/agent/agentcontainers/api.go +++ b/agent/agentcontainers/api.go @@ -8,6 +8,7 @@ import ( "path" "slices" "strings" + "sync" "time" "github.com/fsnotify/fsnotify" @@ -25,35 +26,35 @@ import ( ) const ( - defaultGetContainersCacheDuration = 10 * time.Second - dockerCreatedAtTimeFormat = "2006-01-02 15:04:05 -0700 MST" - getContainersTimeout = 5 * time.Second + defaultUpdateInterval = 10 * time.Second + listContainersTimeout = 15 * time.Second ) // API is responsible for container-related operations in the agent. // It provides methods to list and manage containers. type API struct { - ctx context.Context - cancel context.CancelFunc - done chan struct{} - logger slog.Logger - watcher watcher.Watcher - - cacheDuration time.Duration - execer agentexec.Execer - cl Lister - dccli DevcontainerCLI - clock quartz.Clock - scriptLogger func(logSourceID uuid.UUID) ScriptLogger - - // lockCh protects the below fields. We use a channel instead of a - // mutex so we can handle cancellation properly. - lockCh chan struct{} - containers codersdk.WorkspaceAgentListContainersResponse - mtime time.Time - devcontainerNames map[string]struct{} // Track devcontainer names to avoid duplicates. - knownDevcontainers []codersdk.WorkspaceAgentDevcontainer // Track predefined and runtime-detected devcontainers. - configFileModifiedTimes map[string]time.Time // Track when config files were last modified. + ctx context.Context + cancel context.CancelFunc + watcherDone chan struct{} + updaterDone chan struct{} + initialUpdateDone chan struct{} // Closed after first update in updaterLoop. + updateTrigger chan chan error // Channel to trigger manual refresh. + updateInterval time.Duration // Interval for periodic container updates. + logger slog.Logger + watcher watcher.Watcher + execer agentexec.Execer + cl Lister + dccli DevcontainerCLI + clock quartz.Clock + scriptLogger func(logSourceID uuid.UUID) ScriptLogger + + mu sync.RWMutex + closed bool + containers codersdk.WorkspaceAgentListContainersResponse // Output from the last list operation. + containersErr error // Error from the last list operation. + devcontainerNames map[string]struct{} + knownDevcontainers []codersdk.WorkspaceAgentDevcontainer + configFileModifiedTimes map[string]time.Time devcontainerLogSourceIDs map[string]uuid.UUID // Track devcontainer log source IDs. } @@ -69,15 +70,6 @@ func WithClock(clock quartz.Clock) Option { } } -// WithCacheDuration sets the cache duration for the API. -// This is used to control how often the API refreshes the list of -// containers. The default is 10 seconds. -func WithCacheDuration(d time.Duration) Option { - return func(api *API) { - api.cacheDuration = d - } -} - // WithExecer sets the agentexec.Execer implementation to use. func WithExecer(execer agentexec.Execer) Option { return func(api *API) { @@ -169,12 +161,14 @@ func NewAPI(logger slog.Logger, options ...Option) *API { api := &API{ ctx: ctx, cancel: cancel, - done: make(chan struct{}), + watcherDone: make(chan struct{}), + updaterDone: make(chan struct{}), + initialUpdateDone: make(chan struct{}), + updateTrigger: make(chan chan error), + updateInterval: defaultUpdateInterval, logger: logger, clock: quartz.NewReal(), execer: agentexec.DefaultExecer, - cacheDuration: defaultGetContainersCacheDuration, - lockCh: make(chan struct{}, 1), devcontainerNames: make(map[string]struct{}), knownDevcontainers: []codersdk.WorkspaceAgentDevcontainer{}, configFileModifiedTimes: make(map[string]time.Time), @@ -200,33 +194,16 @@ func NewAPI(logger slog.Logger, options ...Option) *API { } } - go api.loop() + go api.watcherLoop() + go api.updaterLoop() return api } -// SignalReady signals the API that we are ready to begin watching for -// file changes. This is used to prime the cache with the current list -// of containers and to start watching the devcontainer config files for -// changes. It should be called after the agent ready. -func (api *API) SignalReady() { - // Prime the cache with the current list of containers. - _, _ = api.cl.List(api.ctx) - - // Make sure we watch the devcontainer config files for changes. - for _, devcontainer := range api.knownDevcontainers { - if devcontainer.ConfigPath == "" { - continue - } - - if err := api.watcher.Add(devcontainer.ConfigPath); err != nil { - api.logger.Error(api.ctx, "watch devcontainer config file failed", slog.Error(err), slog.F("file", devcontainer.ConfigPath)) - } - } -} - -func (api *API) loop() { - defer close(api.done) +func (api *API) watcherLoop() { + defer close(api.watcherDone) + defer api.logger.Debug(api.ctx, "watcher loop stopped") + api.logger.Debug(api.ctx, "watcher loop started") for { event, err := api.watcher.Next(api.ctx) @@ -263,10 +240,94 @@ func (api *API) loop() { } } +// updaterLoop is responsible for periodically updating the container +// list and handling manual refresh requests. +func (api *API) updaterLoop() { + defer close(api.updaterDone) + defer api.logger.Debug(api.ctx, "updater loop stopped") + api.logger.Debug(api.ctx, "updater loop started") + + // Perform an initial update to populate the container list, this + // gives us a guarantee that the API has loaded the initial state + // before returning any responses. This is useful for both tests + // and anyone looking to interact with the API. + api.logger.Debug(api.ctx, "performing initial containers update") + if err := api.updateContainers(api.ctx); err != nil { + api.logger.Error(api.ctx, "initial containers update failed", slog.Error(err)) + } else { + api.logger.Debug(api.ctx, "initial containers update complete") + } + // Signal that the initial update attempt (successful or not) is done. + // Other services can wait on this if they need the first data to be available. + close(api.initialUpdateDone) + + // We utilize a TickerFunc here instead of a regular Ticker so that + // we can guarantee execution of the updateContainers method after + // advancing the clock. + ticker := api.clock.TickerFunc(api.ctx, api.updateInterval, func() error { + done := make(chan error, 1) + defer close(done) + + select { + case <-api.ctx.Done(): + return api.ctx.Err() + case api.updateTrigger <- done: + err := <-done + if err != nil { + api.logger.Error(api.ctx, "updater loop ticker failed", slog.Error(err)) + } + default: + api.logger.Debug(api.ctx, "updater loop ticker skipped, update in progress") + } + + return nil // Always nil to keep the ticker going. + }, "updaterLoop") + defer func() { + if err := ticker.Wait("updaterLoop"); err != nil && !errors.Is(err, context.Canceled) { + api.logger.Error(api.ctx, "updater loop ticker failed", slog.Error(err)) + } + }() + + for { + select { + case <-api.ctx.Done(): + return + case done := <-api.updateTrigger: + // Note that although we pass api.ctx here, updateContainers + // has an internal timeout to prevent long blocking calls. + done <- api.updateContainers(api.ctx) + } + } +} + // Routes returns the HTTP handler for container-related routes. func (api *API) Routes() http.Handler { r := chi.NewRouter() + ensureInitialUpdateDoneMW := func(next http.Handler) http.Handler { + return http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { + select { + case <-api.ctx.Done(): + httpapi.Write(r.Context(), rw, http.StatusServiceUnavailable, codersdk.Response{ + Message: "API closed", + Detail: "The API is closed and cannot process requests.", + }) + return + case <-r.Context().Done(): + return + case <-api.initialUpdateDone: + // Initial update is done, we can start processing + // requests. + } + next.ServeHTTP(rw, r) + }) + } + + // For now, all endpoints require the initial update to be done. + // If we want to allow some endpoints to be available before + // the initial update, we can enable this per-route. + r.Use(ensureInitialUpdateDoneMW) + r.Get("/", api.handleList) r.Route("/devcontainers", func(r chi.Router) { r.Get("/", api.handleDevcontainersList) @@ -278,62 +339,53 @@ func (api *API) Routes() http.Handler { // handleList handles the HTTP request to list containers. func (api *API) handleList(rw http.ResponseWriter, r *http.Request) { - select { - case <-r.Context().Done(): - // Client went away. + ct, err := api.getContainers() + if err != nil { + httpapi.Write(r.Context(), rw, http.StatusInternalServerError, codersdk.Response{ + Message: "Could not get containers", + Detail: err.Error(), + }) return - default: - ct, err := api.getContainers(r.Context()) - if err != nil { - if errors.Is(err, context.Canceled) { - httpapi.Write(r.Context(), rw, http.StatusRequestTimeout, codersdk.Response{ - Message: "Could not get containers.", - Detail: "Took too long to list containers.", - }) - return - } - httpapi.Write(r.Context(), rw, http.StatusInternalServerError, codersdk.Response{ - Message: "Could not get containers.", - Detail: err.Error(), - }) - return - } - - httpapi.Write(r.Context(), rw, http.StatusOK, ct) } + httpapi.Write(r.Context(), rw, http.StatusOK, ct) } -func copyListContainersResponse(resp codersdk.WorkspaceAgentListContainersResponse) codersdk.WorkspaceAgentListContainersResponse { - return codersdk.WorkspaceAgentListContainersResponse{ - Containers: slices.Clone(resp.Containers), - Warnings: slices.Clone(resp.Warnings), - } -} +// updateContainers fetches the latest container list, processes it, and +// updates the cache. It performs locking for updating shared API state. +func (api *API) updateContainers(ctx context.Context) error { + listCtx, listCancel := context.WithTimeout(ctx, listContainersTimeout) + defer listCancel() -func (api *API) getContainers(ctx context.Context) (codersdk.WorkspaceAgentListContainersResponse, error) { - select { - case <-api.ctx.Done(): - return codersdk.WorkspaceAgentListContainersResponse{}, api.ctx.Err() - case <-ctx.Done(): - return codersdk.WorkspaceAgentListContainersResponse{}, ctx.Err() - case api.lockCh <- struct{}{}: - defer func() { <-api.lockCh }() - } + updated, err := api.cl.List(listCtx) + if err != nil { + // If the context was canceled, we hold off on clearing the + // containers cache. This is to avoid clearing the cache if + // the update was canceled due to a timeout. Hopefully this + // will clear up on the next update. + if !errors.Is(err, context.Canceled) { + api.mu.Lock() + api.containers = codersdk.WorkspaceAgentListContainersResponse{} + api.containersErr = err + api.mu.Unlock() + } - now := api.clock.Now() - if now.Sub(api.mtime) < api.cacheDuration { - return copyListContainersResponse(api.containers), nil + return xerrors.Errorf("list containers failed: %w", err) } - timeoutCtx, timeoutCancel := context.WithTimeout(ctx, getContainersTimeout) - defer timeoutCancel() - updated, err := api.cl.List(timeoutCtx) - if err != nil { - return codersdk.WorkspaceAgentListContainersResponse{}, xerrors.Errorf("get containers: %w", err) - } - api.containers = updated - api.mtime = now + api.mu.Lock() + defer api.mu.Unlock() + api.processUpdatedContainersLocked(ctx, updated) + + api.logger.Debug(ctx, "containers updated successfully", slog.F("container_count", len(api.containers.Containers)), slog.F("warning_count", len(api.containers.Warnings)), slog.F("devcontainer_count", len(api.knownDevcontainers))) + + return nil +} + +// processUpdatedContainersLocked updates the devcontainer state based +// on the latest list of containers. This method assumes that api.mu is +// held. +func (api *API) processUpdatedContainersLocked(ctx context.Context, updated codersdk.WorkspaceAgentListContainersResponse) { dirtyStates := make(map[string]bool) // Reset all known devcontainers to not running. for i := range api.knownDevcontainers { @@ -345,6 +397,7 @@ func (api *API) getContainers(ctx context.Context) (codersdk.WorkspaceAgentListC } // Check if the container is running and update the known devcontainers. + updatedDevcontainers := make(map[string]bool) for i := range updated.Containers { container := &updated.Containers[i] workspaceFolder := container.Labels[DevcontainerLocalFolderLabel] @@ -354,18 +407,16 @@ func (api *API) getContainers(ctx context.Context) (codersdk.WorkspaceAgentListC continue } - container.DevcontainerDirty = dirtyStates[workspaceFolder] - if container.DevcontainerDirty { - lastModified, hasModTime := api.configFileModifiedTimes[configFile] - if hasModTime && container.CreatedAt.After(lastModified) { - api.logger.Info(ctx, "new container created after config modification, not marking as dirty", - slog.F("container", container.ID), - slog.F("created_at", container.CreatedAt), - slog.F("config_modified_at", lastModified), - slog.F("file", configFile), - ) - container.DevcontainerDirty = false - } + if lastModified, hasModTime := api.configFileModifiedTimes[configFile]; !hasModTime || container.CreatedAt.Before(lastModified) { + api.logger.Debug(ctx, "container created before config modification, setting dirty state from devcontainer", + slog.F("container", container.ID), + slog.F("created_at", container.CreatedAt), + slog.F("config_modified_at", lastModified), + slog.F("file", configFile), + slog.F("workspace_folder", workspaceFolder), + slog.F("dirty", dirtyStates[workspaceFolder]), + ) + container.DevcontainerDirty = dirtyStates[workspaceFolder] } // Check if this is already in our known list. @@ -373,29 +424,17 @@ func (api *API) getContainers(ctx context.Context) (codersdk.WorkspaceAgentListC return dc.WorkspaceFolder == workspaceFolder }); knownIndex != -1 { // Update existing entry with runtime information. - if configFile != "" && api.knownDevcontainers[knownIndex].ConfigPath == "" { - api.knownDevcontainers[knownIndex].ConfigPath = configFile + dc := &api.knownDevcontainers[knownIndex] + if configFile != "" && dc.ConfigPath == "" { + dc.ConfigPath = configFile if err := api.watcher.Add(configFile); err != nil { api.logger.Error(ctx, "watch devcontainer config file failed", slog.Error(err), slog.F("file", configFile)) } } - api.knownDevcontainers[knownIndex].Running = container.Running - api.knownDevcontainers[knownIndex].Container = container - - // Check if this container was created after the config - // file was modified. - if configFile != "" && api.knownDevcontainers[knownIndex].Dirty { - lastModified, hasModTime := api.configFileModifiedTimes[configFile] - if hasModTime && container.CreatedAt.After(lastModified) { - api.logger.Info(ctx, "clearing dirty flag for container created after config modification", - slog.F("container", container.ID), - slog.F("created_at", container.CreatedAt), - slog.F("config_modified_at", lastModified), - slog.F("file", configFile), - ) - api.knownDevcontainers[knownIndex].Dirty = false - } - } + dc.Running = container.Running + dc.Container = container + dc.Dirty = container.DevcontainerDirty + updatedDevcontainers[workspaceFolder] = true continue } @@ -428,9 +467,73 @@ func (api *API) getContainers(ctx context.Context) (codersdk.WorkspaceAgentListC Dirty: container.DevcontainerDirty, Container: container, }) + updatedDevcontainers[workspaceFolder] = true + } + + for i := range api.knownDevcontainers { + if _, ok := updatedDevcontainers[api.knownDevcontainers[i].WorkspaceFolder]; ok { + continue + } + + dc := &api.knownDevcontainers[i] + + if !dc.Running && !dc.Dirty && dc.Container == nil { + // Already marked as not running, skip. + continue + } + + api.logger.Debug(ctx, "devcontainer is not running anymore, marking as not running", + slog.F("workspace_folder", dc.WorkspaceFolder), + slog.F("config_path", dc.ConfigPath), + slog.F("name", dc.Name), + ) + dc.Running = false + dc.Dirty = false + dc.Container = nil + } + + api.containers = updated + api.containersErr = nil +} + +// refreshContainers triggers an immediate update of the container list +// and waits for it to complete. +func (api *API) refreshContainers(ctx context.Context) (err error) { + defer func() { + if err != nil { + err = xerrors.Errorf("refresh containers failed: %w", err) + } + }() + + done := make(chan error, 1) + select { + case <-api.ctx.Done(): + return xerrors.Errorf("API closed: %w", api.ctx.Err()) + case <-ctx.Done(): + return ctx.Err() + case api.updateTrigger <- done: + select { + case <-api.ctx.Done(): + return xerrors.Errorf("API closed: %w", api.ctx.Err()) + case <-ctx.Done(): + return ctx.Err() + case err := <-done: + return err + } } +} - return copyListContainersResponse(api.containers), nil +func (api *API) getContainers() (codersdk.WorkspaceAgentListContainersResponse, error) { + api.mu.RLock() + defer api.mu.RUnlock() + + if api.containersErr != nil { + return codersdk.WorkspaceAgentListContainersResponse{}, api.containersErr + } + return codersdk.WorkspaceAgentListContainersResponse{ + Containers: slices.Clone(api.containers.Containers), + Warnings: slices.Clone(api.containers.Warnings), + }, nil } // handleDevcontainerRecreate handles the HTTP request to recreate a @@ -447,7 +550,7 @@ func (api *API) handleDevcontainerRecreate(w http.ResponseWriter, r *http.Reques return } - containers, err := api.getContainers(ctx) + containers, err := api.getContainers() if err != nil { httpapi.Write(ctx, w, http.StatusInternalServerError, codersdk.Response{ Message: "Could not list containers", @@ -509,30 +612,9 @@ func (api *API) handleDevcontainerRecreate(w http.ResponseWriter, r *http.Reques return } - // TODO(mafredri): Temporarily handle clearing the dirty state after - // recreation, later on this should be handled by a "container watcher". - if !api.doLockedHandler(w, r, func() { - for i := range api.knownDevcontainers { - if api.knownDevcontainers[i].WorkspaceFolder == workspaceFolder { - if api.knownDevcontainers[i].Dirty { - api.logger.Info(ctx, "clearing dirty flag after recreation", - slog.F("workspace_folder", workspaceFolder), - slog.F("name", api.knownDevcontainers[i].Name), - ) - api.knownDevcontainers[i].Dirty = false - // TODO(mafredri): This should be handled by a service that - // updates the devcontainer state periodically and on-demand. - api.knownDevcontainers[i].Container = nil - // Set the modified time to the zero value to indicate that - // the containers list must be refreshed. This will see to - // it that the new container is re-assigned. - api.mtime = time.Time{} - } - return - } - } - }) { - return + // NOTE(mafredri): This won't be needed once recreation is done async. + if err := api.refreshContainers(r.Context()); err != nil { + api.logger.Error(ctx, "failed to trigger immediate refresh after devcontainer recreation", slog.Error(err)) } w.WriteHeader(http.StatusNoContent) @@ -542,8 +624,10 @@ func (api *API) handleDevcontainerRecreate(w http.ResponseWriter, r *http.Reques func (api *API) handleDevcontainersList(w http.ResponseWriter, r *http.Request) { ctx := r.Context() - // Run getContainers to detect the latest devcontainers and their state. - _, err := api.getContainers(ctx) + api.mu.RLock() + err := api.containersErr + devcontainers := slices.Clone(api.knownDevcontainers) + api.mu.RUnlock() if err != nil { httpapi.Write(ctx, w, http.StatusInternalServerError, codersdk.Response{ Message: "Could not list containers", @@ -552,13 +636,6 @@ func (api *API) handleDevcontainersList(w http.ResponseWriter, r *http.Request) return } - var devcontainers []codersdk.WorkspaceAgentDevcontainer - if !api.doLockedHandler(w, r, func() { - devcontainers = slices.Clone(api.knownDevcontainers) - }) { - return - } - slices.SortFunc(devcontainers, func(a, b codersdk.WorkspaceAgentDevcontainer) int { if cmp := strings.Compare(a.WorkspaceFolder, b.WorkspaceFolder); cmp != 0 { return cmp @@ -576,75 +653,56 @@ func (api *API) handleDevcontainersList(w http.ResponseWriter, r *http.Request) // markDevcontainerDirty finds the devcontainer with the given config file path // and marks it as dirty. It acquires the lock before modifying the state. func (api *API) markDevcontainerDirty(configPath string, modifiedAt time.Time) { - ok := api.doLocked(func() { - // Record the timestamp of when this configuration file was modified. - api.configFileModifiedTimes[configPath] = modifiedAt + api.mu.Lock() + defer api.mu.Unlock() - for i := range api.knownDevcontainers { - if api.knownDevcontainers[i].ConfigPath != configPath { - continue - } + // Record the timestamp of when this configuration file was modified. + api.configFileModifiedTimes[configPath] = modifiedAt - // TODO(mafredri): Simplistic mark for now, we should check if the - // container is running and if the config file was modified after - // the container was created. - if !api.knownDevcontainers[i].Dirty { - api.logger.Info(api.ctx, "marking devcontainer as dirty", - slog.F("file", configPath), - slog.F("name", api.knownDevcontainers[i].Name), - slog.F("workspace_folder", api.knownDevcontainers[i].WorkspaceFolder), - slog.F("modified_at", modifiedAt), - ) - api.knownDevcontainers[i].Dirty = true - if api.knownDevcontainers[i].Container != nil { - api.knownDevcontainers[i].Container.DevcontainerDirty = true - } - } + for i := range api.knownDevcontainers { + dc := &api.knownDevcontainers[i] + if dc.ConfigPath != configPath { + continue } - }) - if !ok { - api.logger.Debug(api.ctx, "mark devcontainer dirty failed", slog.F("file", configPath)) - } -} -func (api *API) doLockedHandler(w http.ResponseWriter, r *http.Request, f func()) bool { - select { - case <-r.Context().Done(): - httpapi.Write(r.Context(), w, http.StatusRequestTimeout, codersdk.Response{ - Message: "Request canceled", - Detail: "Request was canceled before we could process it.", - }) - return false - case <-api.ctx.Done(): - httpapi.Write(r.Context(), w, http.StatusServiceUnavailable, codersdk.Response{ - Message: "API closed", - Detail: "The API is closed and cannot process requests.", - }) - return false - case api.lockCh <- struct{}{}: - defer func() { <-api.lockCh }() + logger := api.logger.With( + slog.F("file", configPath), + slog.F("name", dc.Name), + slog.F("workspace_folder", dc.WorkspaceFolder), + slog.F("modified_at", modifiedAt), + ) + + // TODO(mafredri): Simplistic mark for now, we should check if the + // container is running and if the config file was modified after + // the container was created. + if !dc.Dirty { + logger.Info(api.ctx, "marking devcontainer as dirty") + dc.Dirty = true + } + if dc.Container != nil && !dc.Container.DevcontainerDirty { + logger.Info(api.ctx, "marking devcontainer container as dirty") + dc.Container.DevcontainerDirty = true + } } - f() - return true } -func (api *API) doLocked(f func()) bool { - select { - case <-api.ctx.Done(): - return false - case api.lockCh <- struct{}{}: - defer func() { <-api.lockCh }() +func (api *API) Close() error { + api.mu.Lock() + if api.closed { + api.mu.Unlock() + return nil } - f() - return true -} + api.closed = true + + api.logger.Debug(api.ctx, "closing API") + defer api.logger.Debug(api.ctx, "closed API") -func (api *API) Close() error { api.cancel() - <-api.done err := api.watcher.Close() - if err != nil { - return err - } - return nil + + api.mu.Unlock() + <-api.watcherDone + <-api.updaterDone + + return err } diff --git a/agent/agentcontainers/api_test.go b/agent/agentcontainers/api_test.go index 2e173b7d5a6b4..a687cb8c001f8 100644 --- a/agent/agentcontainers/api_test.go +++ b/agent/agentcontainers/api_test.go @@ -161,15 +161,16 @@ func TestAPI(t *testing.T) { return codersdk.WorkspaceAgentListContainersResponse{Containers: cts} } + type initialDataPayload struct { + val codersdk.WorkspaceAgentListContainersResponse + err error + } + // Each test case is called multiple times to ensure idempotency for _, tc := range []struct { name string - // data to be stored in the handler - cacheData codersdk.WorkspaceAgentListContainersResponse - // duration of cache - cacheDur time.Duration - // relative age of the cached data - cacheAge time.Duration + // initialData to be stored in the handler + initialData initialDataPayload // function to set up expectations for the mock setupMock func(mcl *acmock.MockLister, preReq *gomock.Call) // expected result @@ -178,104 +179,119 @@ func TestAPI(t *testing.T) { expectedErr string }{ { - name: "no cache", + name: "no initial data", + initialData: initialDataPayload{makeResponse(), nil}, setupMock: func(mcl *acmock.MockLister, preReq *gomock.Call) { mcl.EXPECT().List(gomock.Any()).Return(makeResponse(fakeCt), nil).After(preReq).AnyTimes() }, expected: makeResponse(fakeCt), }, { - name: "no data", - cacheData: makeResponse(), - cacheAge: 2 * time.Second, - cacheDur: time.Second, + name: "repeat initial data", + initialData: initialDataPayload{makeResponse(fakeCt), nil}, + expected: makeResponse(fakeCt), + }, + { + name: "lister error always", + initialData: initialDataPayload{makeResponse(), assert.AnError}, + expectedErr: assert.AnError.Error(), + }, + { + name: "lister error only during initial data", + initialData: initialDataPayload{makeResponse(), assert.AnError}, setupMock: func(mcl *acmock.MockLister, preReq *gomock.Call) { mcl.EXPECT().List(gomock.Any()).Return(makeResponse(fakeCt), nil).After(preReq).AnyTimes() }, expected: makeResponse(fakeCt), }, { - name: "cached data", - cacheAge: time.Second, - cacheData: makeResponse(fakeCt), - cacheDur: 2 * time.Second, - expected: makeResponse(fakeCt), - }, - { - name: "lister error", + name: "lister error after initial data", + initialData: initialDataPayload{makeResponse(fakeCt), nil}, setupMock: func(mcl *acmock.MockLister, preReq *gomock.Call) { mcl.EXPECT().List(gomock.Any()).Return(makeResponse(), assert.AnError).After(preReq).AnyTimes() }, expectedErr: assert.AnError.Error(), }, { - name: "stale cache", - cacheAge: 2 * time.Second, - cacheData: makeResponse(fakeCt), - cacheDur: time.Second, + name: "updated data", + initialData: initialDataPayload{makeResponse(fakeCt), nil}, setupMock: func(mcl *acmock.MockLister, preReq *gomock.Call) { mcl.EXPECT().List(gomock.Any()).Return(makeResponse(fakeCt2), nil).After(preReq).AnyTimes() }, expected: makeResponse(fakeCt2), }, } { - tc := tc t.Run(tc.name, func(t *testing.T) { t.Parallel() var ( ctx = testutil.Context(t, testutil.WaitShort) - clk = quartz.NewMock(t) - ctrl = gomock.NewController(t) - mockLister = acmock.NewMockLister(ctrl) - now = time.Now().UTC() - logger = slogtest.Make(t, nil).Leveled(slog.LevelDebug) + mClock = quartz.NewMock(t) + tickerTrap = mClock.Trap().TickerFunc("updaterLoop") + mCtrl = gomock.NewController(t) + mLister = acmock.NewMockLister(mCtrl) + logger = slogtest.Make(t, &slogtest.Options{IgnoreErrors: true}).Leveled(slog.LevelDebug) r = chi.NewRouter() - api = agentcontainers.NewAPI(logger, - agentcontainers.WithCacheDuration(tc.cacheDur), - agentcontainers.WithClock(clk), - agentcontainers.WithLister(mockLister), - ) ) - defer api.Close() - - r.Mount("/", api.Routes()) - preReq := mockLister.EXPECT().List(gomock.Any()).Return(tc.cacheData, nil).Times(1) + initialDataCall := mLister.EXPECT().List(gomock.Any()).Return(tc.initialData.val, tc.initialData.err) if tc.setupMock != nil { - tc.setupMock(mockLister, preReq) + tc.setupMock(mLister, initialDataCall.Times(1)) + } else { + initialDataCall.AnyTimes() } - if tc.cacheAge != 0 { - clk.Set(now.Add(-tc.cacheAge)).MustWait(ctx) + api := agentcontainers.NewAPI(logger, + agentcontainers.WithClock(mClock), + agentcontainers.WithLister(mLister), + ) + defer api.Close() + r.Mount("/", api.Routes()) + + // Make sure the ticker function has been registered + // before advancing the clock. + tickerTrap.MustWait(ctx).Release() + tickerTrap.Close() + + // Initial request returns the initial data. + req := httptest.NewRequest(http.MethodGet, "/", nil). + WithContext(ctx) + rec := httptest.NewRecorder() + r.ServeHTTP(rec, req) + + if tc.initialData.err != nil { + got := &codersdk.Error{} + err := json.NewDecoder(rec.Body).Decode(got) + require.NoError(t, err, "unmarshal response failed") + require.ErrorContains(t, got, tc.initialData.err.Error(), "want error") } else { - clk.Set(now).MustWait(ctx) + var got codersdk.WorkspaceAgentListContainersResponse + err := json.NewDecoder(rec.Body).Decode(&got) + require.NoError(t, err, "unmarshal response failed") + require.Equal(t, tc.initialData.val, got, "want initial data") } - // Prime the cache with the initial data. - req := httptest.NewRequest(http.MethodGet, "/", nil) - rec := httptest.NewRecorder() + // Advance the clock to run updaterLoop. + _, aw := mClock.AdvanceNext() + aw.MustWait(ctx) + + // Second request returns the updated data. + req = httptest.NewRequest(http.MethodGet, "/", nil). + WithContext(ctx) + rec = httptest.NewRecorder() r.ServeHTTP(rec, req) - clk.Set(now).MustWait(ctx) - - // Repeat the test to ensure idempotency - for i := 0; i < 2; i++ { - req = httptest.NewRequest(http.MethodGet, "/", nil) - rec = httptest.NewRecorder() - r.ServeHTTP(rec, req) - - if tc.expectedErr != "" { - got := &codersdk.Error{} - err := json.NewDecoder(rec.Body).Decode(got) - require.NoError(t, err, "unmarshal response failed") - require.ErrorContains(t, got, tc.expectedErr, "expected error (attempt %d)", i) - } else { - var got codersdk.WorkspaceAgentListContainersResponse - err := json.NewDecoder(rec.Body).Decode(&got) - require.NoError(t, err, "unmarshal response failed") - require.Equal(t, tc.expected, got, "expected containers to be equal (attempt %d)", i) - } + if tc.expectedErr != "" { + got := &codersdk.Error{} + err := json.NewDecoder(rec.Body).Decode(got) + require.NoError(t, err, "unmarshal response failed") + require.ErrorContains(t, got, tc.expectedErr, "want error") + return } + + var got codersdk.WorkspaceAgentListContainersResponse + err := json.NewDecoder(rec.Body).Decode(&got) + require.NoError(t, err, "unmarshal response failed") + require.Equal(t, tc.expected, got, "want updated data") }) } }) @@ -380,7 +396,7 @@ func TestAPI(t *testing.T) { t.Run(tt.name, func(t *testing.T) { t.Parallel() - logger := slogtest.Make(t, nil).Leveled(slog.LevelDebug) + logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true}).Leveled(slog.LevelDebug) // Setup router with the handler under test. r := chi.NewRouter() @@ -393,8 +409,11 @@ func TestAPI(t *testing.T) { defer api.Close() r.Mount("/", api.Routes()) + ctx := testutil.Context(t, testutil.WaitShort) + // Simulate HTTP request to the recreate endpoint. - req := httptest.NewRequest(http.MethodPost, "/devcontainers/container/"+tt.containerID+"/recreate", nil) + req := httptest.NewRequest(http.MethodPost, "/devcontainers/container/"+tt.containerID+"/recreate", nil). + WithContext(ctx) rec := httptest.NewRecorder() r.ServeHTTP(rec, req) @@ -688,7 +707,7 @@ func TestAPI(t *testing.T) { t.Run(tt.name, func(t *testing.T) { t.Parallel() - logger := slogtest.Make(t, nil).Leveled(slog.LevelDebug) + logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true}).Leveled(slog.LevelDebug) // Setup router with the handler under test. r := chi.NewRouter() @@ -712,9 +731,13 @@ func TestAPI(t *testing.T) { api := agentcontainers.NewAPI(logger, apiOptions...) defer api.Close() + r.Mount("/", api.Routes()) - req := httptest.NewRequest(http.MethodGet, "/devcontainers", nil) + ctx := testutil.Context(t, testutil.WaitShort) + + req := httptest.NewRequest(http.MethodGet, "/devcontainers", nil). + WithContext(ctx) rec := httptest.NewRecorder() r.ServeHTTP(rec, req) @@ -739,15 +762,110 @@ func TestAPI(t *testing.T) { } }) + t.Run("List devcontainers running then not running", func(t *testing.T) { + t.Parallel() + + container := codersdk.WorkspaceAgentContainer{ + ID: "container-id", + FriendlyName: "container-name", + Running: true, + CreatedAt: time.Now().Add(-1 * time.Minute), + Labels: map[string]string{ + agentcontainers.DevcontainerLocalFolderLabel: "/home/coder/project", + agentcontainers.DevcontainerConfigFileLabel: "/home/coder/project/.devcontainer/devcontainer.json", + }, + } + dc := codersdk.WorkspaceAgentDevcontainer{ + ID: uuid.New(), + Name: "test-devcontainer", + WorkspaceFolder: "/home/coder/project", + ConfigPath: "/home/coder/project/.devcontainer/devcontainer.json", + } + + ctx := testutil.Context(t, testutil.WaitShort) + + logger := slogtest.Make(t, nil).Leveled(slog.LevelDebug) + fLister := &fakeLister{ + containers: codersdk.WorkspaceAgentListContainersResponse{ + Containers: []codersdk.WorkspaceAgentContainer{container}, + }, + } + fWatcher := newFakeWatcher(t) + mClock := quartz.NewMock(t) + mClock.Set(time.Now()).MustWait(ctx) + tickerTrap := mClock.Trap().TickerFunc("updaterLoop") + + api := agentcontainers.NewAPI(logger, + agentcontainers.WithClock(mClock), + agentcontainers.WithLister(fLister), + agentcontainers.WithWatcher(fWatcher), + agentcontainers.WithDevcontainers( + []codersdk.WorkspaceAgentDevcontainer{dc}, + []codersdk.WorkspaceAgentScript{{LogSourceID: uuid.New(), ID: dc.ID}}, + ), + ) + defer api.Close() + + // Make sure the ticker function has been registered + // before advancing any use of mClock.Advance. + tickerTrap.MustWait(ctx).Release() + tickerTrap.Close() + + // Make sure the start loop has been called. + fWatcher.waitNext(ctx) + + // Simulate a file modification event to make the devcontainer dirty. + fWatcher.sendEventWaitNextCalled(ctx, fsnotify.Event{ + Name: "/home/coder/project/.devcontainer/devcontainer.json", + Op: fsnotify.Write, + }) + + // Initially the devcontainer should be running and dirty. + req := httptest.NewRequest(http.MethodGet, "/devcontainers", nil). + WithContext(ctx) + rec := httptest.NewRecorder() + api.Routes().ServeHTTP(rec, req) + + require.Equal(t, http.StatusOK, rec.Code) + var resp1 codersdk.WorkspaceAgentDevcontainersResponse + err := json.NewDecoder(rec.Body).Decode(&resp1) + require.NoError(t, err) + require.Len(t, resp1.Devcontainers, 1) + require.True(t, resp1.Devcontainers[0].Running, "devcontainer should be running initially") + require.True(t, resp1.Devcontainers[0].Dirty, "devcontainer should be dirty initially") + require.NotNil(t, resp1.Devcontainers[0].Container, "devcontainer should have a container initially") + + // Next, simulate a situation where the container is no longer + // running. + fLister.containers.Containers = []codersdk.WorkspaceAgentContainer{} + + // Trigger a refresh which will use the second response from mock + // lister (no containers). + _, aw := mClock.AdvanceNext() + aw.MustWait(ctx) + + // Afterwards the devcontainer should not be running and not dirty. + req = httptest.NewRequest(http.MethodGet, "/devcontainers", nil). + WithContext(ctx) + rec = httptest.NewRecorder() + api.Routes().ServeHTTP(rec, req) + + require.Equal(t, http.StatusOK, rec.Code) + var resp2 codersdk.WorkspaceAgentDevcontainersResponse + err = json.NewDecoder(rec.Body).Decode(&resp2) + require.NoError(t, err) + require.Len(t, resp2.Devcontainers, 1) + require.False(t, resp2.Devcontainers[0].Running, "devcontainer should not be running after empty list") + require.False(t, resp2.Devcontainers[0].Dirty, "devcontainer should not be dirty after empty list") + require.Nil(t, resp2.Devcontainers[0].Container, "devcontainer should not have a container after empty list") + }) + t.Run("FileWatcher", func(t *testing.T) { t.Parallel() - ctx := testutil.Context(t, testutil.WaitMedium) + ctx := testutil.Context(t, testutil.WaitShort) startTime := time.Date(2025, 1, 1, 12, 0, 0, 0, time.UTC) - mClock := quartz.NewMock(t) - mClock.Set(startTime) - fWatcher := newFakeWatcher(t) // Create a fake container with a config file. configPath := "/workspace/project/.devcontainer/devcontainer.json" @@ -762,6 +880,10 @@ func TestAPI(t *testing.T) { }, } + mClock := quartz.NewMock(t) + mClock.Set(startTime) + tickerTrap := mClock.Trap().TickerFunc("updaterLoop") + fWatcher := newFakeWatcher(t) fLister := &fakeLister{ containers: codersdk.WorkspaceAgentListContainersResponse{ Containers: []codersdk.WorkspaceAgentContainer{container}, @@ -777,14 +899,18 @@ func TestAPI(t *testing.T) { ) defer api.Close() - api.SignalReady() - r := chi.NewRouter() r.Mount("/", api.Routes()) + // Make sure the ticker function has been registered + // before advancing any use of mClock.Advance. + tickerTrap.MustWait(ctx).Release() + tickerTrap.Close() + // Call the list endpoint first to ensure config files are // detected and watched. - req := httptest.NewRequest(http.MethodGet, "/devcontainers", nil) + req := httptest.NewRequest(http.MethodGet, "/devcontainers", nil). + WithContext(ctx) rec := httptest.NewRecorder() r.ServeHTTP(rec, req) require.Equal(t, http.StatusOK, rec.Code) @@ -813,10 +939,13 @@ func TestAPI(t *testing.T) { Op: fsnotify.Write, }) - mClock.Advance(time.Minute).MustWait(ctx) + // Advance the clock to run updaterLoop. + _, aw := mClock.AdvanceNext() + aw.MustWait(ctx) // Check if the container is marked as dirty. - req = httptest.NewRequest(http.MethodGet, "/devcontainers", nil) + req = httptest.NewRequest(http.MethodGet, "/devcontainers", nil). + WithContext(ctx) rec = httptest.NewRecorder() r.ServeHTTP(rec, req) require.Equal(t, http.StatusOK, rec.Code) @@ -830,15 +959,18 @@ func TestAPI(t *testing.T) { assert.True(t, response.Devcontainers[0].Container.DevcontainerDirty, "container should be marked as dirty after config file was modified") - mClock.Advance(time.Minute).MustWait(ctx) - container.ID = "new-container-id" // Simulate a new container ID after recreation. container.FriendlyName = "new-container-name" container.CreatedAt = mClock.Now() // Update the creation time. fLister.containers.Containers = []codersdk.WorkspaceAgentContainer{container} + // Advance the clock to run updaterLoop. + _, aw = mClock.AdvanceNext() + aw.MustWait(ctx) + // Check if dirty flag is cleared. - req = httptest.NewRequest(http.MethodGet, "/devcontainers", nil) + req = httptest.NewRequest(http.MethodGet, "/devcontainers", nil). + WithContext(ctx) rec = httptest.NewRecorder() r.ServeHTTP(rec, req) require.Equal(t, http.StatusOK, rec.Code) diff --git a/cli/open_test.go b/cli/open_test.go index 9ba16a32674e2..97d24f0634d9d 100644 --- a/cli/open_test.go +++ b/cli/open_test.go @@ -326,7 +326,7 @@ func TestOpenVSCodeDevContainer(t *testing.T) { }, }, }, nil, - ) + ).AnyTimes() client, workspace, agentToken := setupWorkspaceForAgent(t, func(agents []*proto.Agent) []*proto.Agent { agents[0].Directory = agentDir @@ -501,7 +501,7 @@ func TestOpenVSCodeDevContainer_NoAgentDirectory(t *testing.T) { }, }, }, nil, - ) + ).AnyTimes() client, workspace, agentToken := setupWorkspaceForAgent(t, func(agents []*proto.Agent) []*proto.Agent { agents[0].Name = agentName diff --git a/cli/ssh_test.go b/cli/ssh_test.go index 49f83daa0612a..147fc07372032 100644 --- a/cli/ssh_test.go +++ b/cli/ssh_test.go @@ -2056,12 +2056,6 @@ func TestSSH_Container(t *testing.T) { client, workspace, agentToken := setupWorkspaceForAgent(t) ctrl := gomock.NewController(t) mLister := acmock.NewMockLister(ctrl) - _ = agenttest.New(t, client.URL, agentToken, func(o *agent.Options) { - o.ExperimentalDevcontainersEnabled = true - o.ContainerAPIOptions = append(o.ContainerAPIOptions, agentcontainers.WithLister(mLister)) - }) - _ = coderdtest.NewWorkspaceAgentWaiter(t, client, workspace.ID).Wait() - mLister.EXPECT().List(gomock.Any()).Return(codersdk.WorkspaceAgentListContainersResponse{ Containers: []codersdk.WorkspaceAgentContainer{ { @@ -2070,7 +2064,12 @@ func TestSSH_Container(t *testing.T) { }, }, Warnings: nil, - }, nil) + }, nil).AnyTimes() + _ = agenttest.New(t, client.URL, agentToken, func(o *agent.Options) { + o.ExperimentalDevcontainersEnabled = true + o.ContainerAPIOptions = append(o.ContainerAPIOptions, agentcontainers.WithLister(mLister)) + }) + _ = coderdtest.NewWorkspaceAgentWaiter(t, client, workspace.ID).Wait() cID := uuid.NewString() inv, root := clitest.New(t, "ssh", workspace.Name, "-c", cID) diff --git a/coderd/workspaceagents_test.go b/coderd/workspaceagents_test.go index bd335e20b0fbb..3c829320768e6 100644 --- a/coderd/workspaceagents_test.go +++ b/coderd/workspaceagents_test.go @@ -1295,14 +1295,14 @@ func TestWorkspaceAgentContainers(t *testing.T) { { name: "test response", setupMock: func(mcl *acmock.MockLister) (codersdk.WorkspaceAgentListContainersResponse, error) { - mcl.EXPECT().List(gomock.Any()).Return(testResponse, nil).Times(1) + mcl.EXPECT().List(gomock.Any()).Return(testResponse, nil).AnyTimes() return testResponse, nil }, }, { name: "error response", setupMock: func(mcl *acmock.MockLister) (codersdk.WorkspaceAgentListContainersResponse, error) { - mcl.EXPECT().List(gomock.Any()).Return(codersdk.WorkspaceAgentListContainersResponse{}, assert.AnError).Times(1) + mcl.EXPECT().List(gomock.Any()).Return(codersdk.WorkspaceAgentListContainersResponse{}, assert.AnError).AnyTimes() return codersdk.WorkspaceAgentListContainersResponse{}, assert.AnError }, }, @@ -1314,7 +1314,10 @@ func TestWorkspaceAgentContainers(t *testing.T) { ctrl := gomock.NewController(t) mcl := acmock.NewMockLister(ctrl) expected, expectedErr := tc.setupMock(mcl) - client, db := coderdtest.NewWithDatabase(t, &coderdtest.Options{}) + logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true}).Leveled(slog.LevelDebug) + client, db := coderdtest.NewWithDatabase(t, &coderdtest.Options{ + Logger: &logger, + }) user := coderdtest.CreateFirstUser(t, client) r := dbfake.WorkspaceBuild(t, db, database.WorkspaceTable{ OrganizationID: user.OrganizationID, @@ -1323,6 +1326,7 @@ func TestWorkspaceAgentContainers(t *testing.T) { return agents }).Do() _ = agenttest.New(t, client.URL, r.AgentToken, func(o *agent.Options) { + o.Logger = logger.Named("agent") o.ExperimentalDevcontainersEnabled = true o.ContainerAPIOptions = append(o.ContainerAPIOptions, agentcontainers.WithLister(mcl)) }) @@ -1392,7 +1396,7 @@ func TestWorkspaceAgentRecreateDevcontainer(t *testing.T) { setupMock: func(mcl *acmock.MockLister, mdccli *acmock.MockDevcontainerCLI) int { mcl.EXPECT().List(gomock.Any()).Return(codersdk.WorkspaceAgentListContainersResponse{ Containers: []codersdk.WorkspaceAgentContainer{devContainer}, - }, nil).Times(1) + }, nil).AnyTimes() mdccli.EXPECT().Up(gomock.Any(), workspaceFolder, configFile, gomock.Any()).Return("someid", nil).Times(1) return 0 }, @@ -1400,7 +1404,7 @@ func TestWorkspaceAgentRecreateDevcontainer(t *testing.T) { { name: "Container does not exist", setupMock: func(mcl *acmock.MockLister, mdccli *acmock.MockDevcontainerCLI) int { - mcl.EXPECT().List(gomock.Any()).Return(codersdk.WorkspaceAgentListContainersResponse{}, nil).Times(1) + mcl.EXPECT().List(gomock.Any()).Return(codersdk.WorkspaceAgentListContainersResponse{}, nil).AnyTimes() return http.StatusNotFound }, }, @@ -1409,7 +1413,7 @@ func TestWorkspaceAgentRecreateDevcontainer(t *testing.T) { setupMock: func(mcl *acmock.MockLister, mdccli *acmock.MockDevcontainerCLI) int { mcl.EXPECT().List(gomock.Any()).Return(codersdk.WorkspaceAgentListContainersResponse{ Containers: []codersdk.WorkspaceAgentContainer{plainContainer}, - }, nil).Times(1) + }, nil).AnyTimes() return http.StatusNotFound }, }, @@ -1421,7 +1425,10 @@ func TestWorkspaceAgentRecreateDevcontainer(t *testing.T) { mcl := acmock.NewMockLister(ctrl) mdccli := acmock.NewMockDevcontainerCLI(ctrl) wantStatus := tc.setupMock(mcl, mdccli) - client, db := coderdtest.NewWithDatabase(t, &coderdtest.Options{}) + logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true}).Leveled(slog.LevelDebug) + client, db := coderdtest.NewWithDatabase(t, &coderdtest.Options{ + Logger: &logger, + }) user := coderdtest.CreateFirstUser(t, client) r := dbfake.WorkspaceBuild(t, db, database.WorkspaceTable{ OrganizationID: user.OrganizationID, @@ -1430,6 +1437,7 @@ func TestWorkspaceAgentRecreateDevcontainer(t *testing.T) { return agents }).Do() _ = agenttest.New(t, client.URL, r.AgentToken, func(o *agent.Options) { + o.Logger = logger.Named("agent") o.ExperimentalDevcontainersEnabled = true o.ContainerAPIOptions = append( o.ContainerAPIOptions,