feat: show devcontainer dirty status and allow recreate (#17880)

Updates #16424

agent/agentcontainers/acmock/acmock.go

@@ -1,9 +1,9 @@
 // Code generated by MockGen. DO NOT EDIT.
-// Source: .. (interfaces: Lister)
+// Source: .. (interfaces: Lister,DevcontainerCLI)
 //
 // Generated by this command:
 //
-//	mockgen -destination ./acmock.go -package acmock .. Lister
+//	mockgen -destination ./acmock.go -package acmock .. Lister,DevcontainerCLI
 //
 
 // Package acmock is a generated GoMock package.
@@ -13,6 +13,7 @@ import (
 	context "context"
 	reflect "reflect"
 
+	agentcontainers "github.com/coder/coder/v2/agent/agentcontainers"
 	codersdk "github.com/coder/coder/v2/codersdk"
 	gomock "go.uber.org/mock/gomock"
 )
@@ -55,3 +56,47 @@ func (mr *MockListerMockRecorder) List(ctx any) *gomock.Call {
 	mr.mock.ctrl.T.Helper()
 	return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "List", reflect.TypeOf((*MockLister)(nil).List), ctx)
 }
+
+// MockDevcontainerCLI is a mock of DevcontainerCLI interface.
+type MockDevcontainerCLI struct {
+	ctrl     *gomock.Controller
+	recorder *MockDevcontainerCLIMockRecorder
+	isgomock struct{}
+}
+
+// MockDevcontainerCLIMockRecorder is the mock recorder for MockDevcontainerCLI.
+type MockDevcontainerCLIMockRecorder struct {
+	mock *MockDevcontainerCLI
+}
+
+// NewMockDevcontainerCLI creates a new mock instance.
+func NewMockDevcontainerCLI(ctrl *gomock.Controller) *MockDevcontainerCLI {
+	mock := &MockDevcontainerCLI{ctrl: ctrl}
+	mock.recorder = &MockDevcontainerCLIMockRecorder{mock}
+	return mock
+}
+
+// EXPECT returns an object that allows the caller to indicate expected use.
+func (m *MockDevcontainerCLI) EXPECT() *MockDevcontainerCLIMockRecorder {
+	return m.recorder
+}
+
+// Up mocks base method.
+func (m *MockDevcontainerCLI) Up(ctx context.Context, workspaceFolder, configPath string, opts ...agentcontainers.DevcontainerCLIUpOptions) (string, error) {
+	m.ctrl.T.Helper()
+	varargs := []any{ctx, workspaceFolder, configPath}
+	for _, a := range opts {
+		varargs = append(varargs, a)
+	}
+	ret := m.ctrl.Call(m, "Up", varargs...)
+	ret0, _ := ret[0].(string)
+	ret1, _ := ret[1].(error)
+	return ret0, ret1
+}
+
+// Up indicates an expected call of Up.
+func (mr *MockDevcontainerCLIMockRecorder) Up(ctx, workspaceFolder, configPath any, opts ...any) *gomock.Call {
+	mr.mock.ctrl.T.Helper()
+	varargs := append([]any{ctx, workspaceFolder, configPath}, opts...)
+	return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Up", reflect.TypeOf((*MockDevcontainerCLI)(nil).Up), varargs...)
+}

agent/agentcontainers/acmock/doc.go

@@ -1,4 +1,4 @@
 // Package acmock contains a mock implementation of agentcontainers.Lister for use in tests.
 package acmock
 
-//go:generate mockgen -destination ./acmock.go -package acmock .. Lister
+//go:generate mockgen -destination ./acmock.go -package acmock .. Lister,DevcontainerCLI

agent/agentcontainers/api.go

@@ -69,6 +69,15 @@ 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) {
@@ -336,7 +345,8 @@ func (api *API) getContainers(ctx context.Context) (codersdk.WorkspaceAgentListC
 	}
 
 	// Check if the container is running and update the known devcontainers.
-	for _, container := range updated.Containers {
+	for i := range updated.Containers {
+		container := &updated.Containers[i]
 		workspaceFolder := container.Labels[DevcontainerLocalFolderLabel]
 		configFile := container.Labels[DevcontainerConfigFileLabel]
 
@@ -344,6 +354,20 @@ 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
+			}
+		}
+
 		// Check if this is already in our known list.
 		if knownIndex := slices.IndexFunc(api.knownDevcontainers, func(dc codersdk.WorkspaceAgentDevcontainer) bool {
 			return dc.WorkspaceFolder == workspaceFolder
@@ -356,7 +380,7 @@ func (api *API) getContainers(ctx context.Context) (codersdk.WorkspaceAgentListC
 				}
 			}
 			api.knownDevcontainers[knownIndex].Running = container.Running
-			api.knownDevcontainers[knownIndex].Container = &container
+			api.knownDevcontainers[knownIndex].Container = container
 
 			// Check if this container was created after the config
 			// file was modified.
@@ -395,28 +419,14 @@ func (api *API) getContainers(ctx context.Context) (codersdk.WorkspaceAgentListC
 			}
 		}
 
-		dirty := dirtyStates[workspaceFolder]
-		if dirty {
-			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),
-				)
-				dirty = false
-			}
-		}
-
 		api.knownDevcontainers = append(api.knownDevcontainers, codersdk.WorkspaceAgentDevcontainer{
 			ID:              uuid.New(),
 			Name:            name,
 			WorkspaceFolder: workspaceFolder,
 			ConfigPath:      configFile,
 			Running:         container.Running,
-			Dirty:           dirty,
-			Container:       &container,
+			Dirty:           container.DevcontainerDirty,
+			Container:       container,
 		})
 	}
 
@@ -510,6 +520,13 @@ func (api *API) handleDevcontainerRecreate(w http.ResponseWriter, r *http.Reques
 						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
 			}
@@ -579,6 +596,9 @@ func (api *API) markDevcontainerDirty(configPath string, modifiedAt time.Time) {
 					slog.F("modified_at", modifiedAt),
 				)
 				api.knownDevcontainers[i].Dirty = true
+				if api.knownDevcontainers[i].Container != nil {
+					api.knownDevcontainers[i].Container.DevcontainerDirty = true
+				}
 			}
 		}
 	})

agent/agentcontainers/api_internal_test.go

@@ -1,163 +0,0 @@
-package agentcontainers
-
-import (
-	"math/rand"
-	"strings"
-	"testing"
-	"time"
-
-	"github.com/google/uuid"
-	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
-	"go.uber.org/mock/gomock"
-
-	"cdr.dev/slog"
-	"cdr.dev/slog/sloggers/slogtest"
-	"github.com/coder/coder/v2/agent/agentcontainers/acmock"
-	"github.com/coder/coder/v2/codersdk"
-	"github.com/coder/coder/v2/testutil"
-	"github.com/coder/quartz"
-)
-
-func TestAPI(t *testing.T) {
-	t.Parallel()
-
-	// List tests the API.getContainers method using a mock
-	// implementation. It specifically tests caching behavior.
-	t.Run("List", func(t *testing.T) {
-		t.Parallel()
-
-		fakeCt := fakeContainer(t)
-		fakeCt2 := fakeContainer(t)
-		makeResponse := func(cts ...codersdk.WorkspaceAgentContainer) codersdk.WorkspaceAgentListContainersResponse {
-			return codersdk.WorkspaceAgentListContainersResponse{Containers: cts}
-		}
-
-		// 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
-			// function to set up expectations for the mock
-			setupMock func(*acmock.MockLister)
-			// expected result
-			expected codersdk.WorkspaceAgentListContainersResponse
-			// expected error
-			expectedErr string
-		}{
-			{
-				name: "no cache",
-				setupMock: func(mcl *acmock.MockLister) {
-					mcl.EXPECT().List(gomock.Any()).Return(makeResponse(fakeCt), nil).AnyTimes()
-				},
-				expected: makeResponse(fakeCt),
-			},
-			{
-				name:      "no data",
-				cacheData: makeResponse(),
-				cacheAge:  2 * time.Second,
-				cacheDur:  time.Second,
-				setupMock: func(mcl *acmock.MockLister) {
-					mcl.EXPECT().List(gomock.Any()).Return(makeResponse(fakeCt), nil).AnyTimes()
-				},
-				expected: makeResponse(fakeCt),
-			},
-			{
-				name:      "cached data",
-				cacheAge:  time.Second,
-				cacheData: makeResponse(fakeCt),
-				cacheDur:  2 * time.Second,
-				expected:  makeResponse(fakeCt),
-			},
-			{
-				name: "lister error",
-				setupMock: func(mcl *acmock.MockLister) {
-					mcl.EXPECT().List(gomock.Any()).Return(makeResponse(), assert.AnError).AnyTimes()
-				},
-				expectedErr: assert.AnError.Error(),
-			},
-			{
-				name:      "stale cache",
-				cacheAge:  2 * time.Second,
-				cacheData: makeResponse(fakeCt),
-				cacheDur:  time.Second,
-				setupMock: func(mcl *acmock.MockLister) {
-					mcl.EXPECT().List(gomock.Any()).Return(makeResponse(fakeCt2), nil).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)
-					api        = NewAPI(logger, WithLister(mockLister))
-				)
-				defer api.Close()
-
-				api.cacheDuration = tc.cacheDur
-				api.clock = clk
-				api.containers = tc.cacheData
-				if tc.cacheAge != 0 {
-					api.mtime = now.Add(-tc.cacheAge)
-				}
-				if tc.setupMock != nil {
-					tc.setupMock(mockLister)
-				}
-
-				clk.Set(now).MustWait(ctx)
-
-				// Repeat the test to ensure idempotency
-				for i := 0; i < 2; i++ {
-					actual, err := api.getContainers(ctx)
-					if tc.expectedErr != "" {
-						require.Empty(t, actual, "expected no data (attempt %d)", i)
-						require.ErrorContains(t, err, tc.expectedErr, "expected error (attempt %d)", i)
-					} else {
-						require.NoError(t, err, "expected no error (attempt %d)", i)
-						require.Equal(t, tc.expected, actual, "expected containers to be equal (attempt %d)", i)
-					}
-				}
-			})
-		}
-	})
-}
-
-func fakeContainer(t *testing.T, mut ...func(*codersdk.WorkspaceAgentContainer)) codersdk.WorkspaceAgentContainer {
-	t.Helper()
-	ct := codersdk.WorkspaceAgentContainer{
-		CreatedAt:    time.Now().UTC(),
-		ID:           uuid.New().String(),
-		FriendlyName: testutil.GetRandomName(t),
-		Image:        testutil.GetRandomName(t) + ":" + strings.Split(uuid.New().String(), "-")[0],
-		Labels: map[string]string{
-			testutil.GetRandomName(t): testutil.GetRandomName(t),
-		},
-		Running: true,
-		Ports: []codersdk.WorkspaceAgentContainerPort{
-			{
-				Network:  "tcp",
-				Port:     testutil.RandomPortNoListen(t),
-				HostPort: testutil.RandomPortNoListen(t),
-				//nolint:gosec // this is a test
-				HostIP: []string{"127.0.0.1", "[::1]", "localhost", "0.0.0.0", "[::]", testutil.GetRandomName(t)}[rand.Intn(6)],
-			},
-		},
-		Status:  testutil.MustRandString(t, 10),
-		Volumes: map[string]string{testutil.GetRandomName(t): testutil.GetRandomName(t)},
-	}
-	for _, m := range mut {
-		m(&ct)
-	}
-	return ct
-}

agent/agentcontainers/api_test.go

@@ -3,8 +3,10 @@ package agentcontainers_test
 import (
 	"context"
 	"encoding/json"
+	"math/rand"
 	"net/http"
 	"net/http/httptest"
+	"strings"
 	"testing"
 	"time"
 
@@ -13,11 +15,13 @@ import (
 	"github.com/google/uuid"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+	"go.uber.org/mock/gomock"
 	"golang.org/x/xerrors"
 
 	"cdr.dev/slog"
 	"cdr.dev/slog/sloggers/slogtest"
 	"github.com/coder/coder/v2/agent/agentcontainers"
+	"github.com/coder/coder/v2/agent/agentcontainers/acmock"
 	"github.com/coder/coder/v2/agent/agentcontainers/watcher"
 	"github.com/coder/coder/v2/codersdk"
 	"github.com/coder/coder/v2/testutil"
@@ -146,6 +150,136 @@ func (w *fakeWatcher) sendEventWaitNextCalled(ctx context.Context, event fsnotif
 func TestAPI(t *testing.T) {
 	t.Parallel()
 
+	// List tests the API.getContainers method using a mock
+	// implementation. It specifically tests caching behavior.
+	t.Run("List", func(t *testing.T) {
+		t.Parallel()
+
+		fakeCt := fakeContainer(t)
+		fakeCt2 := fakeContainer(t)
+		makeResponse := func(cts ...codersdk.WorkspaceAgentContainer) codersdk.WorkspaceAgentListContainersResponse {
+			return codersdk.WorkspaceAgentListContainersResponse{Containers: cts}
+		}
+
+		// 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
+			// function to set up expectations for the mock
+			setupMock func(mcl *acmock.MockLister, preReq *gomock.Call)
+			// expected result
+			expected codersdk.WorkspaceAgentListContainersResponse
+			// expected error
+			expectedErr string
+		}{
+			{
+				name: "no cache",
+				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,
+				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",
+				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,
+				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)
+					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)
+				if tc.setupMock != nil {
+					tc.setupMock(mockLister, preReq)
+				}
+
+				if tc.cacheAge != 0 {
+					clk.Set(now.Add(-tc.cacheAge)).MustWait(ctx)
+				} else {
+					clk.Set(now).MustWait(ctx)
+				}
+
+				// Prime the cache with the initial data.
+				req := httptest.NewRequest(http.MethodGet, "/", nil)
+				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)
+					}
+				}
+			})
+		}
+	})
+
 	t.Run("Recreate", func(t *testing.T) {
 		t.Parallel()
 
@@ -660,6 +794,9 @@ func TestAPI(t *testing.T) {
 		require.NoError(t, err)
 		require.Len(t, response.Devcontainers, 1)
 		assert.False(t, response.Devcontainers[0].Dirty,
+			"devcontainer should not be marked as dirty initially")
+		require.NotNil(t, response.Devcontainers[0].Container, "container should not be nil")
+		assert.False(t, response.Devcontainers[0].Container.DevcontainerDirty,
 			"container should not be marked as dirty initially")
 
 		// Verify the watcher is watching the config file.
@@ -689,6 +826,9 @@ func TestAPI(t *testing.T) {
 		require.Len(t, response.Devcontainers, 1)
 		assert.True(t, response.Devcontainers[0].Dirty,
 			"container should be marked as dirty after config file was modified")
+		require.NotNil(t, response.Devcontainers[0].Container, "container should not be nil")
+		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)
 
@@ -707,7 +847,10 @@ func TestAPI(t *testing.T) {
 		require.NoError(t, err)
 		require.Len(t, response.Devcontainers, 1)
 		assert.False(t, response.Devcontainers[0].Dirty,
-			"dirty flag should be cleared after container recreation")
+			"dirty flag should be cleared on the devcontainer after container recreation")
+		require.NotNil(t, response.Devcontainers[0].Container, "container should not be nil")
+		assert.False(t, response.Devcontainers[0].Container.DevcontainerDirty,
+			"dirty flag should be cleared on the container after container recreation")
 	})
 }
 
@@ -725,3 +868,32 @@ func mustFindDevcontainerByPath(t *testing.T, devcontainers []codersdk.Workspace
 	require.Failf(t, "no devcontainer found with workspace folder %q", path)
 	return codersdk.WorkspaceAgentDevcontainer{} // Unreachable, but required for compilation
 }
+
+func fakeContainer(t *testing.T, mut ...func(*codersdk.WorkspaceAgentContainer)) codersdk.WorkspaceAgentContainer {
+	t.Helper()
+	ct := codersdk.WorkspaceAgentContainer{
+		CreatedAt:    time.Now().UTC(),
+		ID:           uuid.New().String(),
+		FriendlyName: testutil.GetRandomName(t),
+		Image:        testutil.GetRandomName(t) + ":" + strings.Split(uuid.New().String(), "-")[0],
+		Labels: map[string]string{
+			testutil.GetRandomName(t): testutil.GetRandomName(t),
+		},
+		Running: true,
+		Ports: []codersdk.WorkspaceAgentContainerPort{
+			{
+				Network:  "tcp",
+				Port:     testutil.RandomPortNoListen(t),
+				HostPort: testutil.RandomPortNoListen(t),
+				//nolint:gosec // this is a test
+				HostIP: []string{"127.0.0.1", "[::1]", "localhost", "0.0.0.0", "[::]", testutil.GetRandomName(t)}[rand.Intn(6)],
+			},
+		},
+		Status:  testutil.MustRandString(t, 10),
+		Volumes: map[string]string{testutil.GetRandomName(t): testutil.GetRandomName(t)},
+	}
+	for _, m := range mut {
+		m(&ct)
+	}
+	return ct
+}