fix: close two workspace-delete volume leaks

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-<id>` 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).