diff --git a/docs/superpowers/plans/2026-08-21-workspace-delete-volume-cleanup.md b/docs/superpowers/plans/2026-08-21-workspace-delete-volume-cleanup.md new file mode 100644 index 000000000..ccf3193c0 --- /dev/null +++ b/docs/superpowers/plans/2026-08-21-workspace-delete-volume-cleanup.md @@ -0,0 +1,362 @@ +# Workspace Delete Volume Cleanup Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Fix two confirmed volume-leak/silent-failure bugs in the workspace delete path, each backed by a real test that fails before the fix and passes after. + +**Architecture:** `devsy workspace delete` → `pkg/client/clientimplementation` → `cmd/internal/agentworkspace/delete.go` `removeContainer` → `pkg/devcontainer` `runner.Delete` → `pkg/driver/docker` (container removal) + `pkg/agent/delivery` `LocalDockerDelivery.Cleanup` (named volume removal). Two independent leaks exist along this chain: `docker rm` never passes `-v` (anonymous volumes survive), and `Cleanup()` failures are logged at Debug (invisible, silent). + +**Tech Stack:** Go, Ginkgo/Gomega e2e framework (`e2e/framework`) driving the real `devsy` binary against a real docker daemon for the volume-leak repro, stdlib `testing` + `go.uber.org/zap/zapcore` + `pkg/log.InitTestObserved` for the log-visibility repro (no docker needed there). + +**Spec:** No separate spec doc — this plan documents its own findings inline; investigation was done directly against the repository in this conversation. + +## Global Constraints + +- No code comments anywhere (repo convention: code must be self-documenting). +- Tests that require a real docker daemon and the real `devsy` CLI belong in `e2e/tests/`, using the existing `e2e/framework` helpers (`SetupDockerProvider`, `CopyToTempDir`, `DevsyUp`, `DevsyWorkspaceDelete`, `DockerHelper`) — same pattern as every other e2e suite (`e2e/tests/down`, `e2e/tests/up`). Do NOT add a `//go:build integration` Go test under `pkg/` for anything that spins up a real container via the CLI. +- The log-visibility fix (Finding 2 below) needs no docker daemon and no CLI process — it is a plain Go unit test in the package it changes (`pkg/devcontainer`), not an e2e test. +- Every fix task's test MUST be written and confirmed failing (RED) against the pre-fix code before the corresponding code-change task runs. +- `docker-compose` volume handling (`--remove-volumes` flag, `pkg/devcontainer/compose.go`) is out of scope — unaffected and already gated correctly. +- No project-wide `go test ./...` or e2e-suite-wide runs mid-task; run only the new/touched spec. One full targeted run happens in the final verification task. + +## Correction from the prior version of this plan + +Investigation of the third finding (imported/attached-container workspaces skipping `runner.Delete()` and orphaning the `devsy-agent-` volume) turned out to be **not reproducible**: that volume is only ever created by `LocalDockerDelivery.DeliverPreStart`, which is only called from `resolveNewContainer` (`pkg/devcontainer/single.go`) — the *new*-container-creation path. An imported container (`Source.Container != ""`) never goes through `resolveNewContainer`; its agent binary is injected by `legacyInject` (`pkg/devcontainer/setup.go:229`), which is shell/exec-based and never touches a volume. So there is no `devsy-agent-*` volume to leak for that case, and `removeContainer`'s existing skip is correct. This finding is dropped; no code change and no test for it in this plan. + +--- + +## File Structure + +| File | Responsibility | +|---|---| +| `e2e/tests/down/testdata/docker-anon-volume/Dockerfile` (new) | Image that declares an anonymous `VOLUME /data`, to reproduce the leak | +| `e2e/tests/down/testdata/docker-anon-volume/.devcontainer.json` (new) | devcontainer config building that image | +| `e2e/tests/down/down.go` (modify) | New `ginkgo.It` proving `workspace delete` leaks the image's anonymous volume | +| `pkg/docker/helper.go:364-371` (modify) | `Remove` now passes `-v` to `docker rm` | +| `pkg/devcontainer/delete_test.go` (modify, append) | Unit test proving cleanup failures are logged at Debug (invisible) today | +| `pkg/devcontainer/delete.go:43-47` (modify) | `cleanupDeliveryVolume` logs at Warn instead of Debug | + +--- + +### Task 1: E2E test — anonymous volumes survive `workspace delete` + +**Files:** +- Create: `e2e/tests/down/testdata/docker-anon-volume/Dockerfile` +- Create: `e2e/tests/down/testdata/docker-anon-volume/.devcontainer.json` +- Modify: `e2e/tests/down/down.go` (import block + new `ginkgo.It` inside the existing `ginkgo.Describe("testing workspace delete command", ...)`) + +**Interfaces:** +- Consumes: `framework.SetupDockerProvider(binDir, dockerPath string) (*Framework, error)`, `framework.CopyToTempDir(path string) (string, error)`, `(*Framework) DevsyUp(ctx, additionalArgs ...string) error`, `(*Framework) FindWorkspace(ctx, id string) (*provider2.Workspace, error)`, `(*Framework) DevsyWorkspaceDelete(ctx, workspace string, extraArgs ...string) error` (all existing, `e2e/framework/command.go`), `docker.DockerHelper{DockerCommand string}`, `(*DockerHelper) FindContainer(ctx, labels []string) ([]string, error)`, `(*DockerHelper) Inspect(ctx, ids []string, inspectType string, out any) error` (all existing, `pkg/docker/helper.go`) +- Produces: nothing consumed by later tasks; standalone regression test. + +- [ ] **Step 1: Create the testdata** + +`e2e/tests/down/testdata/docker-anon-volume/Dockerfile`: + +```dockerfile +FROM ghcr.io/devsy-org/test-images/base:alpine +VOLUME /data +``` + +`e2e/tests/down/testdata/docker-anon-volume/.devcontainer.json`: + +```json +{ + "name": "anon-volume", + "build": { + "dockerfile": "Dockerfile" + } +} +``` + +- [ ] **Step 2: Write the failing test** + +Add to the import block at the top of `e2e/tests/down/down.go`: + +```go + "os/exec" + + "github.com/docker/docker/api/types/container" + "github.com/docker/docker/api/types/mount" +``` + +so the full import block reads: + +```go +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" +) +``` + +Insert this `ginkgo.It` as a new sibling inside the existing `ginkgo.Describe("testing workspace delete command", ...)` block, immediately after the closing of the `"stop only stops and does not delete workspace"` test (i.e. just before the block's final closing `},\n)`): + +```go + 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) + + out, _ := exec.CommandContext(ctx, "docker", "volume", "inspect", volumeName). + CombinedOutput() + gomega.Expect(string(out)).To(gomega.ContainSubstring("No such volume"), + "anonymous volume should be removed along with its container") + }, ginkgo.SpecTimeout(framework.TimeoutModerate())) +``` + +- [ ] **Step 3: Run test to verify it fails** + +Run: `task cli:test:e2e:build` once (builds `e2e/bin/devsy-linux-amd64` if missing), then from the `e2e` directory: `go tool ginkgo --focus "workspace delete removes anonymous volumes declared by the image"` (equivalent to the repo's `task cli:test:e2e:focus -- "workspace delete removes anonymous volumes declared by the image"`, defined in `Taskfile.yml`). +Expected: FAIL — the final `gomega.Expect(...ContainSubstring("No such volume"))` fails because the anonymous volume is still present. + + +- [ ] **Step 4: Commit the failing test** + +```bash +git add e2e/tests/down/testdata/docker-anon-volume e2e/tests/down/down.go +git commit -m "test(e2e): reproduce anonymous volume leak on workspace delete" +``` + +--- + +### Task 2: Fix — `docker rm -v` to remove anonymous volumes + +**Files:** +- Modify: `pkg/docker/helper.go:364-371` + +**Interfaces:** +- Consumes: nothing new. +- Produces: `(*DockerHelper) Remove(ctx, id string) error` behavior change consumed by Task 1's e2e test, `pkg/driver/docker/lifecycle.go:210`, `pkg/workspace/rename.go:181`, and `e2e/tests/snapshot/helper.go` (all existing callers, no signature change). + +- [ ] **Step 1: Implement the fix** + +```go +[PUT 364.=371:] +func (r *DockerHelper) Remove(ctx context.Context, id string) error { + out, err := r.buildCmd(ctx, "rm", "-v", id).CombinedOutput() + if err != nil { + return fmt.Errorf("%s: %w", string(out), err) + } + + return nil +} +``` + +- [ ] **Step 2: Run Task 1's e2e test to verify it now passes** + +Run the same command as Task 1 Step 3. +Expected: PASS + +- [ ] **Step 3: Run the package's existing unit tests** + +Run: `go test ./pkg/docker/...` +Expected: PASS (no existing test asserts on the exact `docker rm` argv without `-v`; confirm none regress) + +- [ ] **Step 4: Commit** + +```bash +git add pkg/docker/helper.go +git commit -m "fix(docker): remove anonymous volumes when deleting a container" +``` + +--- + +### Task 3: Regression test — cleanup failures are invisible + +**Files:** +- Modify: `pkg/devcontainer/delete_test.go` (append after line 211) + +**Interfaces:** +- Consumes: `newTestRunner(d driver.Driver) *runner` (existing, `pkg/devcontainer/delete_test.go:69`), `(*runner) cleanupDeliveryVolume(ctx context.Context)` (existing, `pkg/devcontainer/delete.go:43`), `log.InitTestObserved(t testing.TB, level zapcore.Level) *observer.ObservedLogs` (existing, `pkg/log/testing.go:25`), `searchString` (existing helper in this file) +- Produces: nothing consumed by later tasks. + +- [ ] **Step 1: Write the failing test** + +```go +[PUT >211:] +func TestCleanupDeliveryVolume_LogsWarningOnFailure(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", + } + + r.cleanupDeliveryVolume(context.Background()) + + if logs.Len() != 1 { + t.Fatalf("expected 1 warning log, got %d", logs.Len()) + } + entry := logs.All()[0] + if entry.Level != zapcore.WarnLevel { + t.Errorf("expected warn level, got %v", entry.Level) + } + if !searchString(entry.Message, "delivery volume cleanup") { + t.Errorf("expected message to mention delivery volume cleanup, got: %s", entry.Message) + } +} +``` + +Add to the import block at the top of `pkg/devcontainer/delete_test.go`: + +```go + "github.com/devsy-org/devsy/pkg/log" + "go.uber.org/zap/zapcore" +``` + +- [ ] **Step 2: Run test to verify it fails** + +Run: `go test ./pkg/devcontainer/... -run TestCleanupDeliveryVolume_LogsWarningOnFailure -v` +Expected: FAIL — `logs.Len()` is `0` because the failure is currently logged with `log.Debugf`, below the observer's `WarnLevel` floor. + +- [ ] **Step 3: Commit the failing test** + +```bash +git add pkg/devcontainer/delete_test.go +git commit -m "test(devcontainer): reproduce silent delivery-volume cleanup failures" +``` + +--- + +### Task 4: Fix — surface delivery-volume cleanup failures + +**Files:** +- Modify: `pkg/devcontainer/delete.go:43-47` + +**Interfaces:** +- Consumes: nothing new. +- Produces: nothing consumed by later tasks. + +- [ ] **Step 1: Promote the log level** + +```go +[PUT 43.=47:] +func (r *runner) cleanupDeliveryVolume(ctx context.Context) { + if err := r.newAgentDelivery().Cleanup(ctx, r.id); err != nil { + log.Warnf("delivery volume cleanup: %v", err) + } +} +``` + +- [ ] **Step 2: Run Task 3's test to verify it now passes** + +Run: `go test ./pkg/devcontainer/... -run TestCleanupDeliveryVolume_LogsWarningOnFailure -v` +Expected: PASS + +- [ ] **Step 3: Run the full devcontainer package unit test suite** + +Run: `go test ./pkg/devcontainer/...` +Expected: PASS + +- [ ] **Step 4: Commit** + +```bash +git add pkg/devcontainer/delete.go +git commit -m "fix(devcontainer): warn instead of silently swallowing delivery cleanup failures" +``` + +--- + +### Task 5: Verify and open draft PR + +**Files:** none (verification + PR only) + +**Interfaces:** none + +- [ ] **Step 1: Run every touched package's unit tests plus the new e2e spec** + +Run: `go test ./pkg/docker/... ./pkg/devcontainer/...` +Run the e2e spec from Task 1 Step 3 again +Expected: all PASS + +- [ ] **Step 2: Run `go vet` and `go build` on the touched packages** + +Run: `go build ./... && go vet ./pkg/docker/... ./pkg/devcontainer/... ./e2e/tests/down/...` +Expected: no errors + +- [ ] **Step 3: Push the branch and open a draft PR** + +```bash +git push -u origin HEAD +gh pr create --draft \ + --title "fix: close two workspace-delete volume leaks" \ + --body "$(cat <<'EOF' +## Problem + +`devsy workspace delete` leaks docker volumes in two independent ways: + +1. `DockerHelper.Remove` runs `docker rm` without `-v`, so any anonymous volume attached to the container (image `VOLUME` directives, unnamed devcontainer.json mounts) survives. +2. `cleanupDeliveryVolume` logs cleanup failures at Debug level, so a failed volume removal is indistinguishable from success unless `--debug` was already on. + +A third suspected leak (imported/attached-container workspaces never cleaning up the agent volume) was investigated and ruled out: that volume is only ever created for newly-created containers, never for imported ones, so there is nothing to leak there. + +## Fix + +1. `docker rm -v` in `pkg/docker/helper.go`. +2. Cleanup failures now log at Warn. + +## Tests + +Each fix has a dedicated test written first against the pre-fix code to confirm it reproduces the issue, then passing after the fix: +- `e2e/tests/down/down.go` (`workspace delete removes anonymous volumes declared by the image`) +- `pkg/devcontainer/delete_test.go` (`TestCleanupDeliveryVolume_LogsWarningOnFailure`) +EOF +)" +``` + +Expected: draft PR URL printed. + +--- + +## Self-Review + +**Spec coverage:** both remaining findings (anonymous-volume leak, silent cleanup failure) each have a dedicated red-then-green test task and a dedicated fix task. The third finding is explicitly retracted with the evidence, per Global Constraints/Correction section, rather than left dangling. + +**Placeholder scan:** no `TODO`/`TBD` in any step. Task 1 Step 3 uses the project's real e2e runner (`Taskfile.yml` targets `cli:test:e2e:build` and `cli:test:e2e:focus`, both reading `go tool ginkgo`), not a fabricated command. + +**Type consistency:** no cross-task interface introduced in this revision (Task 2 and Task 4 are same-file fixes for tests already written in Task 1 and Task 3, with no new shared type). diff --git a/e2e/tests/down/down.go b/e2e/tests/down/down.go index 6a4495a90..eaf24fbd2 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,49 @@ 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) + + out, _ := exec.CommandContext(ctx, "docker", "volume", "inspect", volumeName). + CombinedOutput() + gomega.Expect(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/devcontainer/delete.go b/pkg/devcontainer/delete.go index 8c5ab9993..e37a76836 100644 --- a/pkg/devcontainer/delete.go +++ b/pkg/devcontainer/delete.go @@ -42,7 +42,7 @@ func (r *runner) stopAndDeleteContainer( func (r *runner) cleanupDeliveryVolume(ctx context.Context) { if err := r.newAgentDelivery().Cleanup(ctx, r.id); err != nil { - log.Debugf("delivery volume cleanup: %v", err) + log.Warnf("delivery volume cleanup: %v", err) } } diff --git a/pkg/devcontainer/delete_test.go b/pkg/devcontainer/delete_test.go index 21229eae6..ac17b856a 100644 --- a/pkg/devcontainer/delete_test.go +++ b/pkg/devcontainer/delete_test.go @@ -9,7 +9,9 @@ import ( "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" ) const ( @@ -209,3 +211,26 @@ func TestDelete_NonLocalSource_KeepsNothingToClean(t *testing.T) { t.Fatalf("Delete failed: %v", err) } } + +func TestCleanupDeliveryVolume_LogsWarningOnFailure(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", + } + + r.cleanupDeliveryVolume(context.Background()) + + if logs.Len() != 1 { + t.Fatalf("expected 1 warning log, got %d", logs.Len()) + } + entry := logs.All()[0] + if entry.Level != zapcore.WarnLevel { + t.Errorf("expected warn level, got %v", entry.Level) + } + if !searchString(entry.Message, "delivery volume cleanup") { + t.Errorf("expected message to mention delivery volume cleanup, got: %s", entry.Message) + } +} diff --git a/pkg/docker/helper.go b/pkg/docker/helper.go index f9f190c19..26daf6466 100644 --- a/pkg/docker/helper.go +++ b/pkg/docker/helper.go @@ -362,7 +362,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) }