From d3d4adb22fc5ccdb4ba52cc23acc51926e051c8d Mon Sep 17 00:00:00 2001 From: Samuel K Date: Tue, 25 Aug 2026 14:54:02 +0000 Subject: [PATCH 1/2] fix(devcontainer): close workspace-delete volume leaks devsy workspace delete leaked docker volumes in two ways: docker rm never passed -v, so anonymous volumes from image VOLUME directives survived, and delivery-volume cleanup failures were logged at debug, indistinguishable from success. Deletion is now an explicit ordered teardown plan with required and best-effort steps: every step runs independently, required failures are aggregated, and best-effort failures warn. Imported workspaces skip only the foreign container while still cleaning devsy-owned leftovers, which the old command-level skip never did; the agent-side delete command aggregates daemon/container errors instead of aborting folder cleanup. LocalDockerDelivery.Cleanup also removes the canonical devsy-agent- volume by name so bootstrap volumes created before management labels existed no longer leak. The --remove-volumes help text now states it removes declared volumes for docker compose workspaces only; devsy-managed volumes are always removed and non-compose deletes log when the flag has no effect. --- cmd/internal/agentworkspace/delete.go | 35 +++--- cmd/workspace/delete.go | 3 +- e2e/tests/down/down.go | 49 ++++++++ .../docker-anon-volume/.devcontainer.json | 6 + .../testdata/docker-anon-volume/Dockerfile | 2 + pkg/agent/delivery/local_docker.go | 11 +- pkg/agent/delivery/local_docker_test.go | 23 ++++ pkg/devcontainer/delete.go | 42 ++----- pkg/devcontainer/delete_test.go | 110 +++++++++++++++++- pkg/devcontainer/teardown.go | 109 +++++++++++++++++ pkg/docker/helper.go | 2 +- 11 files changed, 325 insertions(+), 67 deletions(-) create mode 100644 e2e/tests/down/testdata/docker-anon-volume/.devcontainer.json create mode 100644 e2e/tests/down/testdata/docker-anon-volume/Dockerfile create mode 100644 pkg/devcontainer/teardown.go diff --git a/cmd/internal/agentworkspace/delete.go b/cmd/internal/agentworkspace/delete.go index bd35e68ec..db7171990 100644 --- a/cmd/internal/agentworkspace/delete.go +++ b/cmd/internal/agentworkspace/delete.go @@ -2,6 +2,7 @@ package agentworkspace import ( "context" + "errors" "fmt" "io/fs" "os" @@ -56,7 +57,7 @@ func NewDeleteCmd(flags *flags.GlobalFlags) *cobra.Command { &cmd.RemoveVolumes, names.RemoveVolumes, false, - "Remove named volumes associated with the workspace", + "Remove declared volumes when deleting docker compose workspaces; devsy-managed volumes are always removed", ), cliflags.String(&cmd.WorkspaceInfo, names.WorkspaceInfo, "", "The workspace info"), ) @@ -65,7 +66,6 @@ func NewDeleteCmd(flags *flags.GlobalFlags) *cobra.Command { } func (cmd *DeleteCmd) Run(ctx context.Context) error { - // get workspace shouldExit, workspaceInfo, err := agent.WorkspaceInfo( cmd.WorkspaceInfo, ) @@ -75,25 +75,21 @@ func (cmd *DeleteCmd) Run(ctx context.Context) error { return nil } - // remove daemon + var errs []error if cmd.Daemon { - err = removeDaemon(workspaceInfo) - if err != nil { - return fmt.Errorf("remove daemon: %w", err) + if err := removeDaemon(workspaceInfo); err != nil { + errs = append(errs, fmt.Errorf("remove daemon: %w", err)) } } - - // cleanup docker container if cmd.Container { - err = removeContainer(ctx, workspaceInfo, cmd.RemoveVolumes) - if err != nil { - return fmt.Errorf("remove container: %w", err) + if err := removeContainer(ctx, workspaceInfo, cmd.RemoveVolumes); err != nil { + errs = append(errs, fmt.Errorf("remove container: %w", err)) } } removeWorkspaceFolders(workspaceInfo) - return nil + return errors.Join(errs...) } // removeWorkspaceFolders deletes the workspace's config folder and cached @@ -129,17 +125,12 @@ func removeContainer( return err } - if workspaceInfo.Workspace.Source.Container != "" { - log.Info("skipping container deletion, since it was not created by Devsy") - } else { - err = runner.Delete(ctx, devcontainer.DeleteOptions{ - RemoveVolumes: removeVolumes, - }) - if err != nil { - return err - } - log.Debug("removed Devsy container from server") + if err := runner.Delete(ctx, devcontainer.DeleteOptions{ + RemoveVolumes: removeVolumes, + }); err != nil { + return err } + log.Debug("removed Devsy container from server") return nil } diff --git a/cmd/workspace/delete.go b/cmd/workspace/delete.go index 36ba94992..a541a596a 100644 --- a/cmd/workspace/delete.go +++ b/cmd/workspace/delete.go @@ -59,7 +59,8 @@ Use --ignore-not-found to treat a missing workspace as success.`, cliflags.Bool(&cmd.Force, names.Force, false, "Delete workspace even if it is not found remotely anymore"), cliflags.Bool(&cmd.RemoveVolumes, names.RemoveVolumes, false, - "Remove named volumes associated with the workspace"), + "Remove named volumes associated with the workspace "+ + "(docker compose only); devsy-managed volumes are always removed"), ) return deleteCmd } diff --git a/e2e/tests/down/down.go b/e2e/tests/down/down.go index 6a4495a90..9ba85f6fe 100644 --- a/e2e/tests/down/down.go +++ b/e2e/tests/down/down.go @@ -4,11 +4,14 @@ import ( "context" "fmt" "os" + "os/exec" "strings" "github.com/devsy-org/devsy/e2e/framework" pkgconfig "github.com/devsy-org/devsy/pkg/config" docker "github.com/devsy-org/devsy/pkg/docker" + "github.com/docker/docker/api/types/container" + "github.com/docker/docker/api/types/mount" "github.com/onsi/ginkgo/v2" "github.com/onsi/gomega" ) @@ -138,5 +141,51 @@ var _ = ginkgo.Describe( "container should still exist after stop (only stopped, not deleted)", ) }, ginkgo.SpecTimeout(framework.TimeoutModerate())) + + ginkgo.It("workspace delete removes anonymous volumes declared by the image", + func(ctx context.Context) { + f, err := framework.SetupDockerProvider(initialDir+"/bin", "docker") + framework.ExpectNoError(err) + + tempDir, err := framework.CopyToTempDir("tests/down/testdata/docker-anon-volume") + framework.ExpectNoError(err) + ginkgo.DeferCleanup(framework.CleanupTempDir, initialDir, tempDir) + + err = f.DevsyUp(ctx, tempDir) + framework.ExpectNoError(err) + + workspace, err := f.FindWorkspace(ctx, tempDir) + framework.ExpectNoError(err) + ginkgo.DeferCleanup(f.DevsyWorkspaceDelete, tempDir) + + ids, err := dockerHelper.FindContainer(ctx, []string{ + fmt.Sprintf("%s=%s", pkgconfig.DevcontainerIDLabel, workspace.UID), + }) + framework.ExpectNoError(err) + gomega.Expect(ids).NotTo(gomega.BeEmpty()) + + var details []container.InspectResponse + err = dockerHelper.Inspect(ctx, ids, "container", &details) + framework.ExpectNoError(err) + + var volumeName string + for _, m := range details[0].Mounts { + if m.Type == mount.TypeVolume && m.Destination == "/data" { + volumeName = m.Name + } + } + gomega.Expect(volumeName).NotTo(gomega.BeEmpty(), + "container should have an anonymous volume mounted at /data") + + err = f.DevsyWorkspaceDelete(ctx, tempDir) + framework.ExpectNoError(err) + + // #nosec G204 -- e2e spec with controlled docker binary and volume name + cmd := exec.CommandContext(ctx, "docker", "volume", "inspect", volumeName) + out, _ := cmd.CombinedOutput() + gomega.Expect(strings.ToLower(string(out))).To( + gomega.ContainSubstring("no such volume"), + "anonymous volume should be removed along with its container") + }, ginkgo.SpecTimeout(framework.TimeoutModerate())) }, ) diff --git a/e2e/tests/down/testdata/docker-anon-volume/.devcontainer.json b/e2e/tests/down/testdata/docker-anon-volume/.devcontainer.json new file mode 100644 index 000000000..a9bde41b7 --- /dev/null +++ b/e2e/tests/down/testdata/docker-anon-volume/.devcontainer.json @@ -0,0 +1,6 @@ +{ + "name": "anon-volume", + "build": { + "dockerfile": "Dockerfile" + } +} diff --git a/e2e/tests/down/testdata/docker-anon-volume/Dockerfile b/e2e/tests/down/testdata/docker-anon-volume/Dockerfile new file mode 100644 index 000000000..1f52d8e7f --- /dev/null +++ b/e2e/tests/down/testdata/docker-anon-volume/Dockerfile @@ -0,0 +1,2 @@ +FROM ghcr.io/devsy-org/test-images/base:alpine +VOLUME /data diff --git a/pkg/agent/delivery/local_docker.go b/pkg/agent/delivery/local_docker.go index 8b23e403c..42134eaaa 100644 --- a/pkg/agent/delivery/local_docker.go +++ b/pkg/agent/delivery/local_docker.go @@ -9,6 +9,7 @@ import ( "os" "os/exec" "path/filepath" + "slices" "strings" pkgconfig "github.com/devsy-org/devsy/pkg/config" @@ -75,17 +76,17 @@ func (d *LocalDockerDelivery) DeliverPostStart(_ context.Context, _ PostStartOpt return fmt.Errorf("LocalDockerDelivery does not support post-start delivery") } -// Cleanup removes every devsy-managed volume owned by the workspace (the agent -// volume and any seeded workspace volume), identified by labels. Only labeled -// volumes are removed, so foreign/external volumes are left untouched. +// only labeled devsy-managed volumes and the canonical agent volume are +// removed; foreign volumes are never included. func (d *LocalDockerDelivery) Cleanup(ctx context.Context, workspaceID string) error { volumes, err := d.listManagedVolumes(ctx, workspaceID) if err != nil { return err } - // Attempt every volume so one transient failure does not orphan the rest. var errs []error - for _, name := range volumes { + volumes = append(volumes, volumePrefix+workspaceID) + slices.Sort(volumes) + for _, name := range slices.Compact(volumes) { if err := d.removeVolume(ctx, name); err != nil { errs = append(errs, err) continue diff --git a/pkg/agent/delivery/local_docker_test.go b/pkg/agent/delivery/local_docker_test.go index e42f82f1e..6f20e1ef1 100644 --- a/pkg/agent/delivery/local_docker_test.go +++ b/pkg/agent/delivery/local_docker_test.go @@ -482,6 +482,29 @@ func TestLocalDockerDelivery_Cleanup_RemovesManagedVolumes(t *testing.T) { assert.Contains(t, removed, "ws1-workspace") } +func TestLocalDockerDelivery_Cleanup_RemovesLegacyUnlabeledAgentVolume(t *testing.T) { + tmpDir := t.TempDir() + logPath := filepath.Join(tmpDir, "calls.log") + + scriptPath := filepath.Join(tmpDir, "fake-docker.sh") + script := "#!/bin/sh\n" + + "if [ \"$1\" = \"volume\" ] && [ \"$2\" = \"ls\" ]; then exit 0; fi\n" + + "if [ \"$1\" = \"volume\" ] && [ \"$2\" = \"rm\" ]; then\n" + + " echo \"$@\" >> \"" + logPath + "\"; exit 0\n" + + "fi\n" + + "exit 0\n" + require.NoError(t, os.WriteFile(scriptPath, []byte(script), 0o600)) + // #nosec G302 -- test script must be executable + require.NoError(t, os.Chmod(scriptPath, 0o755)) + + d := &LocalDockerDelivery{DockerCommand: scriptPath} + require.NoError(t, d.Cleanup(context.Background(), "ws-legacy")) + + logged, err := os.ReadFile(logPath) //nolint:gosec // test reads a temp file we control + require.NoError(t, err) + assert.Equal(t, "volume rm -f devsy-agent-ws-legacy\n", string(logged)) +} + func TestLocalDockerDelivery_SeedExcludesBuildInternal(t *testing.T) { tmpDir := t.TempDir() logPath := filepath.Join(tmpDir, "run.log") diff --git a/pkg/devcontainer/delete.go b/pkg/devcontainer/delete.go index a5954579e..177205094 100644 --- a/pkg/devcontainer/delete.go +++ b/pkg/devcontainer/delete.go @@ -9,51 +9,23 @@ import ( ) func (r *runner) Delete(ctx context.Context, options DeleteOptions) error { - containerDetails, err := r.driver.FindDevContainer(ctx, r.id) + details, err := r.findTeardownContainer(ctx) if err != nil { return fmt.Errorf("find dev container: %w", err) } - defer r.cleanupDeliveryVolume(ctx) - defer r.cleanupImportedDevContainer() - if containerDetails == nil { - return nil - } - - log.Infof("deleting devcontainer: devcontainerID=%s", containerDetails.ID) - if isDockerCompose, projectName := getDockerComposeProject(containerDetails); isDockerCompose { - return r.deleteDockerCompose(ctx, projectName, options.RemoveVolumes) + if details != nil { + log.Infof("deleting devcontainer: devcontainerID=%s", details.ID) } - return r.stopAndDeleteContainer(ctx, containerDetails) + return r.buildTeardownPlan(details, options).execute(ctx) } -// stopAndDeleteContainer stops containerDetails' devcontainer if it -// is running, then deletes it. -func (r *runner) stopAndDeleteContainer( - ctx context.Context, containerDetails *config.ContainerDetails, -) error { - if containerDetails.State.Status == config.ContainerStatusRunning { - if err := r.driver.StopDevContainer(ctx, r.id); err != nil { - return err - } - } - return r.driver.DeleteDevContainer(ctx, r.id) -} - -func (r *runner) cleanupDeliveryVolume(ctx context.Context) { - if err := r.newAgentDelivery().Cleanup(ctx, r.id); err != nil { - log.Debugf("delivery volume cleanup: %v", err) - } -} - -func (r *runner) cleanupImportedDevContainer() { +func (r *runner) cleanupImportedDevContainer() error { if r.workspaceConfig == nil || r.workspaceConfig.Workspace == nil || r.workspaceConfig.Workspace.Source.LocalFolder == "" { - return - } - if err := CleanupImportedDevContainers(r.localWorkspaceFolder); err != nil { - log.Debugf("imported devcontainer cleanup: %v", err) + return nil } + return CleanupImportedDevContainers(r.localWorkspaceFolder) } func (r *runner) Stop(ctx context.Context) error { diff --git a/pkg/devcontainer/delete_test.go b/pkg/devcontainer/delete_test.go index 21229eae6..53f74948f 100644 --- a/pkg/devcontainer/delete_test.go +++ b/pkg/devcontainer/delete_test.go @@ -4,12 +4,16 @@ import ( "context" "fmt" "io" + "os" "path/filepath" "testing" "github.com/devsy-org/devsy/pkg/devcontainer/config" "github.com/devsy-org/devsy/pkg/driver" + "github.com/devsy-org/devsy/pkg/log" "github.com/devsy-org/devsy/pkg/provider" + "go.uber.org/zap/zapcore" + "go.uber.org/zap/zaptest/observer" ) const ( @@ -170,11 +174,60 @@ func TestDelete_DeleteError_ReturnsError(t *testing.T) { } } -func TestCleanupDeliveryVolume_DoesNotPanic(t *testing.T) { - d := &mockDriver{} +func TestDelete_ImportedWorkspace_SkipsContainerCleansLeftovers(t *testing.T) { + d := &mockDriver{ + findResult: &config.ContainerDetails{ + ID: testContainerID, + State: config.ContainerDetailsState{Status: testStatusRunning}, + Config: config.ContainerDetailsConfig{Labels: map[string]string{}}, + }, + } r := newTestRunner(d) + r.workspaceConfig.Workspace = &provider.Workspace{ + Source: provider.WorkspaceSource{Container: "foreign-container"}, + } + + if err := r.Delete(context.Background(), DeleteOptions{}); err != nil { + t.Fatalf("Delete failed: %v", err) + } + if d.stopCalled { + t.Error("foreign container must never be stopped") + } + if d.deleteCalled { + t.Error("foreign container must never be deleted") + } +} + +func TestDelete_RequiredFailure_StillRunsLeftoverSteps(t *testing.T) { + ws := t.TempDir() + external := filepath.Join(t.TempDir(), "devcontainer.json") + writeFile(t, external, `{"image":"alpine"}`) + + d := &mockDriver{ + findResult: &config.ContainerDetails{ + ID: testContainerID, + State: config.ContainerDetailsState{Status: "exited"}, + Config: config.ContainerDetailsConfig{Labels: map[string]string{}}, + }, + deleteErr: fmt.Errorf("daemon unreachable"), + } + r := newTestRunner(d) + r.localWorkspaceFolder = ws + r.workspaceConfig.Workspace = &provider.Workspace{ + Source: provider.WorkspaceSource{LocalFolder: ws}, + } - r.cleanupDeliveryVolume(context.Background()) + if _, err := r.importExternalDevContainer(external); err != nil { + t.Fatalf("import failed: %v", err) + } + + err := r.Delete(context.Background(), DeleteOptions{}) + if err == nil { + t.Fatal("expected the container-delete failure to propagate") + } + if dirExists(importedProfilePath(ws)) { + t.Error("leftover cleanup must still run after a required step fails") + } } func TestDelete_RemovesImportedDevContainer(t *testing.T) { @@ -209,3 +262,54 @@ func TestDelete_NonLocalSource_KeepsNothingToClean(t *testing.T) { t.Fatalf("Delete failed: %v", err) } } + +func TestTeardown_DeliveryVolumeFailure_WarnsButSucceeds(t *testing.T) { + logs := log.InitTestObserved(t, zapcore.WarnLevel) + + r := newTestRunner(&mockDriver{}) + r.workspaceConfig.Agent.Driver = provider.DockerDriver + r.workspaceConfig.Agent.Docker = provider.ProviderDockerDriverConfig{ + Path: "devsy-test-nonexistent-docker-binary", + } + + err := r.buildTeardownPlan(nil, DeleteOptions{}).execute(context.Background()) + if err != nil { + t.Fatalf("best-effort failure must not fail teardown, got: %v", err) + } + if !observedWarning(logs.All(), "delivery volume cleanup") { + t.Fatal("expected a warning mentioning delivery volume cleanup") + } +} + +func observedWarning(entries []observer.LoggedEntry, substr string) bool { + for _, e := range entries { + if e.Level == zapcore.WarnLevel && searchString(e.Message, substr) { + return true + } + } + return false +} + +func TestTeardown_ImportedMarkerFailure_WarnsButSucceeds(t *testing.T) { + logs := log.InitTestObserved(t, zapcore.WarnLevel) + + tmpDir := t.TempDir() + notADir := filepath.Join(tmpDir, importedProfileParent) + if err := os.WriteFile(notADir, []byte("x"), 0o600); err != nil { + t.Fatalf("write file: %v", err) + } + + r := newTestRunner(&mockDriver{}) + r.localWorkspaceFolder = tmpDir + r.workspaceConfig.Workspace = &provider.Workspace{ + Source: provider.WorkspaceSource{LocalFolder: tmpDir}, + } + + err := r.buildTeardownPlan(nil, DeleteOptions{}).execute(context.Background()) + if err != nil { + t.Fatalf("best-effort failure must not fail teardown, got: %v", err) + } + if !observedWarning(logs.All(), "imported devcontainer cleanup") { + t.Fatal("expected a warning mentioning imported devcontainer cleanup") + } +} diff --git a/pkg/devcontainer/teardown.go b/pkg/devcontainer/teardown.go new file mode 100644 index 000000000..9b3334bfe --- /dev/null +++ b/pkg/devcontainer/teardown.go @@ -0,0 +1,109 @@ +package devcontainer + +import ( + "context" + "errors" + "fmt" + + "github.com/devsy-org/devsy/pkg/devcontainer/config" + "github.com/devsy-org/devsy/pkg/log" +) + +// Required steps fail teardown when they error; best-effort steps only warn. +type teardownStep struct { + name string + required bool + run func(ctx context.Context) error +} + +// Every step runs independently: one failure never skips later steps. +type teardownPlan struct { + steps []teardownStep +} + +func (t *teardownPlan) add(name string, required bool, run func(ctx context.Context) error) { + t.steps = append(t.steps, teardownStep{name: name, required: required, run: run}) +} + +func (t *teardownPlan) execute(ctx context.Context) error { + var errs []error + for _, step := range t.steps { + err := step.run(ctx) + if err == nil { + continue + } + if !step.required { + log.Warnf("%s: %v", step.name, err) + continue + } + errs = append(errs, fmt.Errorf("%s: %w", step.name, err)) + } + return errors.Join(errs...) +} + +// Nil details means the container is foreign or already gone. +func (r *runner) buildTeardownPlan( + details *config.ContainerDetails, + options DeleteOptions, +) *teardownPlan { + plan := &teardownPlan{} + + if r.isImportedWorkspace() { + log.Info("skipping container deletion, since it was not created by Devsy") + } else if details != nil { + r.addContainerTeardown(plan, details, options) + } + r.addLeftoverTeardown(plan) + + return plan +} + +// Stopped first since runtimes refuse to rm running containers. +func (r *runner) addContainerTeardown( + plan *teardownPlan, + details *config.ContainerDetails, + options DeleteOptions, +) { + if isCompose, projectName := getDockerComposeProject(details); isCompose { + plan.add("docker compose project", true, func(ctx context.Context) error { + return r.deleteDockerCompose(ctx, projectName, options.RemoveVolumes) + }) + return + } + + if options.RemoveVolumes { + log.Infof("--remove-volumes only removes declared volumes for docker compose workspaces") + } + if details.State.Status == config.ContainerStatusRunning { + plan.add("stop devcontainer", true, func(ctx context.Context) error { + return r.driver.StopDevContainer(ctx, r.id) + }) + } + plan.add("delete devcontainer", true, func(ctx context.Context) error { + return r.driver.DeleteDevContainer(ctx, r.id) + }) +} + +func (r *runner) addLeftoverTeardown(plan *teardownPlan) { + plan.add("delivery volume cleanup", false, func(ctx context.Context) error { + return r.newAgentDelivery().Cleanup(ctx, r.id) + }) + plan.add("imported devcontainer cleanup", false, func(ctx context.Context) error { + return r.cleanupImportedDevContainer() + }) +} + +func (r *runner) findTeardownContainer( + ctx context.Context, +) (*config.ContainerDetails, error) { + if r.isImportedWorkspace() { + return nil, nil + } + return r.driver.FindDevContainer(ctx, r.id) +} + +func (r *runner) isImportedWorkspace() bool { + return r.workspaceConfig != nil && + r.workspaceConfig.Workspace != nil && + r.workspaceConfig.Workspace.Source.Container != "" +} diff --git a/pkg/docker/helper.go b/pkg/docker/helper.go index 6d0cfe7ae..78216ada1 100644 --- a/pkg/docker/helper.go +++ b/pkg/docker/helper.go @@ -363,7 +363,7 @@ func (r *DockerHelper) Pull(ctx context.Context, opts PullOptions) error { } func (r *DockerHelper) Remove(ctx context.Context, id string) error { - out, err := r.buildCmd(ctx, "rm", id).CombinedOutput() + out, err := r.buildCmd(ctx, "rm", "-v", id).CombinedOutput() if err != nil { return fmt.Errorf("%s: %w", string(out), err) } From 0efa2d273d3021da76541f2bf48937ca4e9215fa Mon Sep 17 00:00:00 2001 From: Samuel K Date: Tue, 25 Aug 2026 17:16:43 -0500 Subject: [PATCH 2/2] style: update comments --- cmd/internal/agentworkspace/delete.go | 2 +- cmd/workspace/delete.go | 2 +- e2e/tests/down/down.go | 2 +- pkg/agent/delivery/local_docker.go | 2 +- pkg/agent/delivery/local_docker_test.go | 4 ++-- pkg/devcontainer/teardown.go | 6 +----- 6 files changed, 7 insertions(+), 11 deletions(-) diff --git a/cmd/internal/agentworkspace/delete.go b/cmd/internal/agentworkspace/delete.go index db7171990..a1f59c096 100644 --- a/cmd/internal/agentworkspace/delete.go +++ b/cmd/internal/agentworkspace/delete.go @@ -57,7 +57,7 @@ func NewDeleteCmd(flags *flags.GlobalFlags) *cobra.Command { &cmd.RemoveVolumes, names.RemoveVolumes, false, - "Remove declared volumes when deleting docker compose workspaces; devsy-managed volumes are always removed", + "Remove declared volumes when deleting docker compose workspaces", ), cliflags.String(&cmd.WorkspaceInfo, names.WorkspaceInfo, "", "The workspace info"), ) diff --git a/cmd/workspace/delete.go b/cmd/workspace/delete.go index a541a596a..71c997831 100644 --- a/cmd/workspace/delete.go +++ b/cmd/workspace/delete.go @@ -60,7 +60,7 @@ Use --ignore-not-found to treat a missing workspace as success.`, "Delete workspace even if it is not found remotely anymore"), cliflags.Bool(&cmd.RemoveVolumes, names.RemoveVolumes, false, "Remove named volumes associated with the workspace "+ - "(docker compose only); devsy-managed volumes are always removed"), + "(docker compose only)"), ) return deleteCmd } diff --git a/e2e/tests/down/down.go b/e2e/tests/down/down.go index 9ba85f6fe..b9043f53d 100644 --- a/e2e/tests/down/down.go +++ b/e2e/tests/down/down.go @@ -180,7 +180,7 @@ var _ = ginkgo.Describe( err = f.DevsyWorkspaceDelete(ctx, tempDir) framework.ExpectNoError(err) - // #nosec G204 -- e2e spec with controlled docker binary and volume name + // #nosec G204 cmd := exec.CommandContext(ctx, "docker", "volume", "inspect", volumeName) out, _ := cmd.CombinedOutput() gomega.Expect(strings.ToLower(string(out))).To( diff --git a/pkg/agent/delivery/local_docker.go b/pkg/agent/delivery/local_docker.go index 42134eaaa..ad58c2701 100644 --- a/pkg/agent/delivery/local_docker.go +++ b/pkg/agent/delivery/local_docker.go @@ -77,7 +77,7 @@ func (d *LocalDockerDelivery) DeliverPostStart(_ context.Context, _ PostStartOpt } // only labeled devsy-managed volumes and the canonical agent volume are -// removed; foreign volumes are never included. +// removed. func (d *LocalDockerDelivery) Cleanup(ctx context.Context, workspaceID string) error { volumes, err := d.listManagedVolumes(ctx, workspaceID) if err != nil { diff --git a/pkg/agent/delivery/local_docker_test.go b/pkg/agent/delivery/local_docker_test.go index 6f20e1ef1..c2d6f4407 100644 --- a/pkg/agent/delivery/local_docker_test.go +++ b/pkg/agent/delivery/local_docker_test.go @@ -494,13 +494,13 @@ func TestLocalDockerDelivery_Cleanup_RemovesLegacyUnlabeledAgentVolume(t *testin "fi\n" + "exit 0\n" require.NoError(t, os.WriteFile(scriptPath, []byte(script), 0o600)) - // #nosec G302 -- test script must be executable + // #nosec G302 require.NoError(t, os.Chmod(scriptPath, 0o755)) d := &LocalDockerDelivery{DockerCommand: scriptPath} require.NoError(t, d.Cleanup(context.Background(), "ws-legacy")) - logged, err := os.ReadFile(logPath) //nolint:gosec // test reads a temp file we control + logged, err := os.ReadFile(logPath) //nolint:gosec require.NoError(t, err) assert.Equal(t, "volume rm -f devsy-agent-ws-legacy\n", string(logged)) } diff --git a/pkg/devcontainer/teardown.go b/pkg/devcontainer/teardown.go index 9b3334bfe..c3e82e833 100644 --- a/pkg/devcontainer/teardown.go +++ b/pkg/devcontainer/teardown.go @@ -9,14 +9,12 @@ import ( "github.com/devsy-org/devsy/pkg/log" ) -// Required steps fail teardown when they error; best-effort steps only warn. type teardownStep struct { name string required bool run func(ctx context.Context) error } -// Every step runs independently: one failure never skips later steps. type teardownPlan struct { steps []teardownStep } @@ -41,7 +39,6 @@ func (t *teardownPlan) execute(ctx context.Context) error { return errors.Join(errs...) } -// Nil details means the container is foreign or already gone. func (r *runner) buildTeardownPlan( details *config.ContainerDetails, options DeleteOptions, @@ -58,7 +55,6 @@ func (r *runner) buildTeardownPlan( return plan } -// Stopped first since runtimes refuse to rm running containers. func (r *runner) addContainerTeardown( plan *teardownPlan, details *config.ContainerDetails, @@ -72,7 +68,7 @@ func (r *runner) addContainerTeardown( } if options.RemoveVolumes { - log.Infof("--remove-volumes only removes declared volumes for docker compose workspaces") + log.Debugf("--remove-volumes only removes declared volumes for docker compose workspaces") } if details.State.Status == config.ContainerStatusRunning { plan.add("stop devcontainer", true, func(ctx context.Context) error {