From eb9f746deb7569d65d0963508c6c8a38fdd3f562 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Fri, 21 Aug 2026 05:32:29 +0000 Subject: [PATCH 1/7] docs: add workspace delete volume cleanup implementation plan --- ...6-08-21-workspace-delete-volume-cleanup.md | 501 ++++++++++++++++++ 1 file changed, 501 insertions(+) create mode 100644 docs/superpowers/plans/2026-08-21-workspace-delete-volume-cleanup.md 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..3bdab381c --- /dev/null +++ b/docs/superpowers/plans/2026-08-21-workspace-delete-volume-cleanup.md @@ -0,0 +1,501 @@ +# 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 three confirmed volume-leak/silent-failure bugs in the workspace delete path, each backed by a real integration test that fails before the fix and passes after. + +**Architecture:** `devsy workspace delete` → `pkg/client/clientimplementation` (remote exec) → `cmd/internal/agentworkspace/delete.go` `removeContainer` → `pkg/devcontainer` `runner.Delete` → `pkg/driver/docker` (container removal) + `pkg/agent/delivery` `LocalDockerDelivery.Cleanup` (named volume removal). Three independent leaks exist along this chain: `docker rm` never passes `-v` (anonymous volumes survive), the imported-container branch skips the whole `Delete()` call (named agent volume survives), and delivery-cleanup errors are logged at Debug (invisible, silent). + +**Tech Stack:** Go, testify (`require`), stdlib `testing`, `go.uber.org/zap/zapcore` + `pkg/log.InitTestObserved` for log assertions, real `docker` CLI for integration tests (build tag `integration`). + +**Spec:** No separate spec doc — this plan documents its own findings inline (see Global Constraints); investigation was done directly against the repository in this conversation. + +## Global Constraints + +- No code comments anywhere (repo convention: code must be self-documenting). +- Integration tests that require a real docker daemon MUST use `//go:build integration` and skip via a `dockerAvailable()` check, matching `pkg/agent/delivery/delivery_integration_test.go`. +- 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 ./...` runs mid-task; run only the touched packages. One full targeted run happens in the final verification task. + +--- + +## File Structure + +| File | Responsibility | +|---|---| +| `pkg/docker/helper_integration_test.go` (new) | Integration test proving `Remove` leaks anonymous volumes | +| `pkg/docker/helper.go:364-371` (modify) | `Remove` now passes `-v` to `docker rm` | +| `cmd/internal/agentworkspace/delete_integration_test.go` (new) | Integration test proving imported-container workspaces leak the agent volume | +| `pkg/devcontainer/run.go:36-38` (modify) | `DeleteOptions` gains `SkipContainerDelete` | +| `pkg/devcontainer/delete.go:12-47` (modify) | `Delete` honors `SkipContainerDelete`; `cleanupDeliveryVolume` logs at Warn | +| `cmd/internal/agentworkspace/delete.go:121-145` (modify) | `removeContainer` always calls `runner.Delete`, passing `SkipContainerDelete` for imported containers | +| `pkg/devcontainer/delete_test.go` (modify, append) | Unit test proving cleanup failures are logged at Debug (invisible) today | + +--- + +### Task 1: Regression test — anonymous volumes survive `docker rm` + +**Files:** +- Create: `pkg/docker/helper_integration_test.go` + +**Interfaces:** +- Consumes: `DockerHelper{DockerCommand string}`, `(*DockerHelper) Remove(ctx, id string) error` (existing, `pkg/docker/helper.go:364`) +- Produces: nothing consumed by later tasks; this is a standalone regression test. + +- [ ] **Step 1: Write the failing test** + +```go +//go:build integration + +package docker + +import ( + "context" + "os/exec" + "strings" + "testing" + + "github.com/stretchr/testify/require" +) + +func dockerAvailableForHelperTest() bool { + return exec.Command("docker", "info").Run() == nil +} + +func TestDockerHelper_Remove_RemovesAnonymousVolumes(t *testing.T) { + if !dockerAvailableForHelperTest() { + t.Skip("docker not available") + } + + ctx := context.Background() + containerName := "devsy-helper-test-anon-volume" + _ = exec.CommandContext(ctx, "docker", "rm", "-f", containerName).Run() + + out, err := exec.CommandContext(ctx, "docker", "run", "-d", + "--name", containerName, + "-v", "/data", + "alpine:latest", "sleep", "300", + ).CombinedOutput() + require.NoError(t, err, string(out)) + t.Cleanup(func() { + _ = exec.CommandContext(context.Background(), "docker", "rm", "-f", containerName).Run() + }) + + out, err = exec.CommandContext(ctx, "docker", "inspect", + "--format", `{{range .Mounts}}{{if eq .Type "volume"}}{{.Name}}{{end}}{{end}}`, + containerName, + ).CombinedOutput() + require.NoError(t, err, string(out)) + volumeName := strings.TrimSpace(string(out)) + require.NotEmpty(t, volumeName, "container should have an anonymous volume") + + helper := &DockerHelper{DockerCommand: "docker"} + err = helper.Remove(ctx, containerName) + require.NoError(t, err) + + out, _ = exec.CommandContext(ctx, "docker", "volume", "inspect", volumeName).CombinedOutput() + require.Contains(t, string(out), "No such volume", + "anonymous volume should be removed along with its container") +} +``` + +- [ ] **Step 2: Run test to verify it fails** + +Run: `go test -tags=integration ./pkg/docker/... -run TestDockerHelper_Remove_RemovesAnonymousVolumes -v` +Expected: FAIL — `docker volume inspect` still finds the volume, `require.Contains` assertion fails. + +- [ ] **Step 3: Commit the failing test** + +```bash +git add pkg/docker/helper_integration_test.go +git commit -m "test(docker): reproduce anonymous volume leak on container removal" +``` + +--- + +### Task 2: Regression test — imported-container delete skips agent volume cleanup + +**Files:** +- Create: `cmd/internal/agentworkspace/delete_integration_test.go` + +**Interfaces:** +- Consumes: `removeContainer(ctx, workspaceInfo *provider2.AgentWorkspaceInfo, removeVolumes bool) error` (existing, `cmd/internal/agentworkspace/delete.go:121`), `CreateRunner` (existing, `cmd/internal/agentworkspace/up.go:200`) +- Produces: nothing consumed by later tasks; standalone regression test. Calls the exact same `removeContainer` signature the code fix will keep, so no test edits are needed after the fix lands. + +- [ ] **Step 1: Write the failing test** + +```go +//go:build integration + +package agentworkspace + +import ( + "context" + "os/exec" + "strings" + "testing" + + pkgconfig "github.com/devsy-org/devsy/pkg/config" + provider2 "github.com/devsy-org/devsy/pkg/provider" + "github.com/stretchr/testify/require" +) + +func dockerAvailableForDeleteTest() bool { + return exec.Command("docker", "info").Run() == nil +} + +func TestRemoveContainer_ImportedContainer_StillRemovesAgentVolume(t *testing.T) { + if !dockerAvailableForDeleteTest() { + t.Skip("docker not available") + } + + ctx := context.Background() + workspaceID := "test-imported-de-1234" + containerName := "devsy-delete-test-imported-container" + volumeName := "devsy-agent-" + workspaceID + + _ = exec.CommandContext(ctx, "docker", "rm", "-f", containerName).Run() + _ = exec.CommandContext(ctx, "docker", "volume", "rm", "-f", volumeName).Run() + + out, err := exec.CommandContext(ctx, "docker", "run", "-d", + "--name", containerName, + "alpine:latest", "sleep", "300", + ).CombinedOutput() + require.NoError(t, err, string(out)) + t.Cleanup(func() { + _ = exec.CommandContext(context.Background(), "docker", "rm", "-f", containerName).Run() + _ = exec.CommandContext(context.Background(), "docker", "volume", "rm", "-f", volumeName).Run() + }) + + containerID := strings.TrimSpace(string(out)) + + out, err = exec.CommandContext(ctx, "docker", "volume", "create", + "--label", pkgconfig.DockerManagedLabel+"="+pkgconfig.LabelValueTrue, + "--label", pkgconfig.DockerResourceLabel+"="+pkgconfig.ResourceVolume, + "--label", pkgconfig.DockerWorkspaceIDLabel+"="+workspaceID, + "--label", pkgconfig.DockerVolumeRoleLabel+"="+pkgconfig.VolumeRoleAgent, + volumeName, + ).CombinedOutput() + require.NoError(t, err, string(out)) + + workspaceInfo := &provider2.AgentWorkspaceInfo{ + Workspace: &provider2.Workspace{ + ID: workspaceID, + Source: provider2.WorkspaceSource{Container: containerID}, + }, + } + + err = removeContainer(ctx, workspaceInfo, false) + require.NoError(t, err) + + out, _ = exec.CommandContext(ctx, "docker", "inspect", containerID).CombinedOutput() + require.NotContains(t, string(out), "No such object", + "imported container must not be deleted by devsy") + + out, _ = exec.CommandContext(ctx, "docker", "volume", "inspect", volumeName).CombinedOutput() + require.Contains(t, string(out), "No such volume", + "devsy-managed agent volume must be removed even for imported containers") +} +``` + +- [ ] **Step 2: Run test to verify it fails** + +Run: `go test -tags=integration ./cmd/internal/agentworkspace/... -run TestRemoveContainer_ImportedContainer_StillRemovesAgentVolume -v` +Expected: FAIL — the agent volume is still present because `removeContainer` never calls `runner.Delete()` for imported containers. + +- [ ] **Step 3: Commit the failing test** + +```bash +git add cmd/internal/agentworkspace/delete_integration_test.go +git commit -m "test(agentworkspace): reproduce agent volume leak on imported-container delete" +``` + +--- + +### 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 — `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 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 test to verify it now passes** + +Run: `go test -tags=integration ./pkg/docker/... -run TestDockerHelper_Remove_RemovesAnonymousVolumes -v` +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 5: Fix — imported-container delete still cleans up the agent volume + +**Files:** +- Modify: `pkg/devcontainer/run.go:36-38` +- Modify: `pkg/devcontainer/delete.go:12-28` +- Modify: `cmd/internal/agentworkspace/delete.go:121-145` + +**Interfaces:** +- Consumes: nothing new. +- Produces: `DeleteOptions.SkipContainerDelete bool` consumed by `runner.Delete` and by every caller constructing `devcontainer.DeleteOptions`. + +- [ ] **Step 1: Add the option** + +```go +[pkg/devcontainer/run.go PUT 36.=38:] +type DeleteOptions struct { + RemoveVolumes bool + SkipContainerDelete bool +} +``` + +- [ ] **Step 2: Honor the option in `Delete`** + +```go +[pkg/devcontainer/delete.go PUT 12.=21:] +func (r *runner) Delete(ctx context.Context, options DeleteOptions) error { + containerDetails, err := r.driver.FindDevContainer(ctx, r.id) + if err != nil { + return fmt.Errorf("find dev container: %w", err) + } + defer r.cleanupDeliveryVolume(ctx) + defer r.cleanupImportedDevContainer() + if containerDetails == nil || options.SkipContainerDelete { + return nil + } +``` + +- [ ] **Step 3: Always call `runner.Delete`, passing `SkipContainerDelete` instead of skipping the call** + +```go +[cmd/internal/agentworkspace/delete.go PUT 121.=145:] +func removeContainer( + ctx context.Context, + workspaceInfo *provider2.AgentWorkspaceInfo, + removeVolumes bool, +) error { + log.Debugf("removing Devsy container from server: workspaceId=%s", workspaceInfo.Workspace.ID) + runner, err := CreateRunner(ctx, workspaceInfo) + if err != nil { + return err + } + + imported := workspaceInfo.Workspace.Source.Container != "" + if imported { + log.Info("skipping container deletion, since it was not created by Devsy") + } + + if err := runner.Delete(ctx, devcontainer.DeleteOptions{ + RemoveVolumes: removeVolumes, + SkipContainerDelete: imported, + }); err != nil { + return err + } + log.Debug("removed Devsy container from server") + + return nil +} +``` + +- [ ] **Step 4: Run Task 2's test to verify it now passes** + +Run: `go test -tags=integration ./cmd/internal/agentworkspace/... -run TestRemoveContainer_ImportedContainer_StillRemovesAgentVolume -v` +Expected: PASS + +- [ ] **Step 5: Run existing devcontainer and agentworkspace unit tests** + +Run: `go test ./pkg/devcontainer/... ./cmd/internal/agentworkspace/...` +Expected: PASS, including the pre-existing `TestDelete_NilContainer_ReturnsNil`, `TestDelete_RunningContainer_StopsDeletesAndCleansUp`, etc. — none construct `DeleteOptions` with `SkipContainerDelete`, so the zero-value `false` preserves their behavior. + +- [ ] **Step 6: Commit** + +```bash +git add pkg/devcontainer/run.go pkg/devcontainer/delete.go cmd/internal/agentworkspace/delete.go +git commit -m "fix(devcontainer): clean up agent volume for imported-container workspaces" +``` + +--- + +### Task 6: 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 7: Verify and open draft PR + +**Files:** none (verification + PR only) + +**Interfaces:** none + +- [ ] **Step 1: Run every touched package's tests together, including integration tags** + +Run: `go test ./pkg/docker/... ./pkg/devcontainer/... ./cmd/internal/agentworkspace/...` +Run: `go test -tags=integration ./pkg/docker/... ./cmd/internal/agentworkspace/... -run 'TestDockerHelper_Remove_RemovesAnonymousVolumes|TestRemoveContainer_ImportedContainer_StillRemovesAgentVolume' -v` +Expected: all PASS + +- [ ] **Step 2: Run `go vet` and `go build` on the touched packages** + +Run: `go build ./... && go vet ./pkg/docker/... ./pkg/devcontainer/... ./cmd/internal/agentworkspace/...` +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 three workspace-delete volume leaks" \ + --body "$(cat <<'EOF' +## Problem + +`devsy workspace delete` leaks docker volumes in three 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. `removeContainer` skips the entire `runner.Delete()` call for workspaces attached to an imported/pre-existing container (`Source.Container != ""`), so the devsy-managed `devsy-agent-` volume is never cleaned up for those workspaces. +3. `cleanupDeliveryVolume` logs cleanup failures at Debug level, so a failed volume removal is indistinguishable from success unless `--debug` was already on. + +## Fix + +1. `docker rm -v` in `pkg/docker/helper.go`. +2. `DeleteOptions.SkipContainerDelete` lets `removeContainer` keep skipping *container* removal for imported containers while still running the deferred volume cleanup inside `runner.Delete`. +3. Cleanup failures now log at Warn. + +## Tests + +Each fix has a dedicated integration/unit test written first against the pre-fix code to confirm it reproduces the leak, then passing after the fix: +- `pkg/docker/helper_integration_test.go` +- `cmd/internal/agentworkspace/delete_integration_test.go` +- `pkg/devcontainer/delete_test.go` (`TestCleanupDeliveryVolume_LogsWarningOnFailure`) +EOF +)" +``` + +Expected: draft PR URL printed. + +--- + +## Self-Review + +**Spec coverage:** all three findings from the investigation (anonymous-volume leak, imported-container skip, silent cleanup failure) each have a dedicated red-then-green test task and a dedicated fix task. No finding is left uncovered. + +**Placeholder scan:** no `TODO`/`TBD` in any step; every code block is complete and directly derived from the read source lines cited. + +**Type consistency:** `DeleteOptions.SkipContainerDelete` is introduced once (Task 5, Step 1) and consumed with the same field name in `pkg/devcontainer/delete.go` and `cmd/internal/agentworkspace/delete.go`. `removeContainer`'s signature (`ctx, *provider2.AgentWorkspaceInfo, bool`) is unchanged between Task 2's test and Task 5's fix, so the test needs no follow-up edits. From 7daa633b6b334f8e8871d7a2b46854301cd718ea Mon Sep 17 00:00:00 2001 From: Samuel K Date: Fri, 21 Aug 2026 05:37:41 +0000 Subject: [PATCH 2/7] docs: move volume-cleanup repro tests into e2e, retract imported-container finding --- ...6-08-21-workspace-delete-volume-cleanup.md | 441 ++++++------------ 1 file changed, 151 insertions(+), 290 deletions(-) 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 index 3bdab381c..ccf3193c0 100644 --- a/docs/superpowers/plans/2026-08-21-workspace-delete-volume-cleanup.md +++ b/docs/superpowers/plans/2026-08-21-workspace-delete-volume-cleanup.md @@ -2,21 +2,26 @@ > **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 three confirmed volume-leak/silent-failure bugs in the workspace delete path, each backed by a real integration test that fails before the fix and passes after. +**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` (remote exec) → `cmd/internal/agentworkspace/delete.go` `removeContainer` → `pkg/devcontainer` `runner.Delete` → `pkg/driver/docker` (container removal) + `pkg/agent/delivery` `LocalDockerDelivery.Cleanup` (named volume removal). Three independent leaks exist along this chain: `docker rm` never passes `-v` (anonymous volumes survive), the imported-container branch skips the whole `Delete()` call (named agent volume survives), and delivery-cleanup errors are logged at Debug (invisible, silent). +**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, testify (`require`), stdlib `testing`, `go.uber.org/zap/zapcore` + `pkg/log.InitTestObserved` for log assertions, real `docker` CLI for integration tests (build tag `integration`). +**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 (see Global Constraints); investigation was done directly against the repository in this conversation. +**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). -- Integration tests that require a real docker daemon MUST use `//go:build integration` and skip via a `dockerAvailable()` check, matching `pkg/agent/delivery/delivery_integration_test.go`. +- 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 ./...` runs mid-task; run only the touched packages. One full targeted run happens in the final verification task. +- 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. --- @@ -24,261 +29,148 @@ | File | Responsibility | |---|---| -| `pkg/docker/helper_integration_test.go` (new) | Integration test proving `Remove` leaks anonymous volumes | +| `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` | -| `cmd/internal/agentworkspace/delete_integration_test.go` (new) | Integration test proving imported-container workspaces leak the agent volume | -| `pkg/devcontainer/run.go:36-38` (modify) | `DeleteOptions` gains `SkipContainerDelete` | -| `pkg/devcontainer/delete.go:12-47` (modify) | `Delete` honors `SkipContainerDelete`; `cleanupDeliveryVolume` logs at Warn | -| `cmd/internal/agentworkspace/delete.go:121-145` (modify) | `removeContainer` always calls `runner.Delete`, passing `SkipContainerDelete` for imported containers | | `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: Regression test — anonymous volumes survive `docker rm` +### Task 1: E2E test — anonymous volumes survive `workspace delete` **Files:** -- Create: `pkg/docker/helper_integration_test.go` +- 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: `DockerHelper{DockerCommand string}`, `(*DockerHelper) Remove(ctx, id string) error` (existing, `pkg/docker/helper.go:364`) -- Produces: nothing consumed by later tasks; this is a standalone regression test. +- 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: Write the failing test** - -```go -//go:build integration - -package docker - -import ( - "context" - "os/exec" - "strings" - "testing" +- [ ] **Step 1: Create the testdata** - "github.com/stretchr/testify/require" -) +`e2e/tests/down/testdata/docker-anon-volume/Dockerfile`: -func dockerAvailableForHelperTest() bool { - return exec.Command("docker", "info").Run() == nil -} +```dockerfile +FROM ghcr.io/devsy-org/test-images/base:alpine +VOLUME /data +``` -func TestDockerHelper_Remove_RemovesAnonymousVolumes(t *testing.T) { - if !dockerAvailableForHelperTest() { - t.Skip("docker not available") - } +`e2e/tests/down/testdata/docker-anon-volume/.devcontainer.json`: - ctx := context.Background() - containerName := "devsy-helper-test-anon-volume" - _ = exec.CommandContext(ctx, "docker", "rm", "-f", containerName).Run() - - out, err := exec.CommandContext(ctx, "docker", "run", "-d", - "--name", containerName, - "-v", "/data", - "alpine:latest", "sleep", "300", - ).CombinedOutput() - require.NoError(t, err, string(out)) - t.Cleanup(func() { - _ = exec.CommandContext(context.Background(), "docker", "rm", "-f", containerName).Run() - }) - - out, err = exec.CommandContext(ctx, "docker", "inspect", - "--format", `{{range .Mounts}}{{if eq .Type "volume"}}{{.Name}}{{end}}{{end}}`, - containerName, - ).CombinedOutput() - require.NoError(t, err, string(out)) - volumeName := strings.TrimSpace(string(out)) - require.NotEmpty(t, volumeName, "container should have an anonymous volume") - - helper := &DockerHelper{DockerCommand: "docker"} - err = helper.Remove(ctx, containerName) - require.NoError(t, err) - - out, _ = exec.CommandContext(ctx, "docker", "volume", "inspect", volumeName).CombinedOutput() - require.Contains(t, string(out), "No such volume", - "anonymous volume should be removed along with its container") +```json +{ + "name": "anon-volume", + "build": { + "dockerfile": "Dockerfile" + } } ``` -- [ ] **Step 2: Run test to verify it fails** +- [ ] **Step 2: Write the failing test** -Run: `go test -tags=integration ./pkg/docker/... -run TestDockerHelper_Remove_RemovesAnonymousVolumes -v` -Expected: FAIL — `docker volume inspect` still finds the volume, `require.Contains` assertion fails. +Add to the import block at the top of `e2e/tests/down/down.go`: -- [ ] **Step 3: Commit the failing test** +```go + "os/exec" -```bash -git add pkg/docker/helper_integration_test.go -git commit -m "test(docker): reproduce anonymous volume leak on container removal" + "github.com/docker/docker/api/types/container" + "github.com/docker/docker/api/types/mount" ``` ---- - -### Task 2: Regression test — imported-container delete skips agent volume cleanup - -**Files:** -- Create: `cmd/internal/agentworkspace/delete_integration_test.go` - -**Interfaces:** -- Consumes: `removeContainer(ctx, workspaceInfo *provider2.AgentWorkspaceInfo, removeVolumes bool) error` (existing, `cmd/internal/agentworkspace/delete.go:121`), `CreateRunner` (existing, `cmd/internal/agentworkspace/up.go:200`) -- Produces: nothing consumed by later tasks; standalone regression test. Calls the exact same `removeContainer` signature the code fix will keep, so no test edits are needed after the fix lands. - -- [ ] **Step 1: Write the failing test** +so the full import block reads: ```go -//go:build integration - -package agentworkspace - import ( "context" + "fmt" + "os" "os/exec" "strings" - "testing" + "github.com/devsy-org/devsy/e2e/framework" pkgconfig "github.com/devsy-org/devsy/pkg/config" - provider2 "github.com/devsy-org/devsy/pkg/provider" - "github.com/stretchr/testify/require" + 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" ) - -func dockerAvailableForDeleteTest() bool { - return exec.Command("docker", "info").Run() == nil -} - -func TestRemoveContainer_ImportedContainer_StillRemovesAgentVolume(t *testing.T) { - if !dockerAvailableForDeleteTest() { - t.Skip("docker not available") - } - - ctx := context.Background() - workspaceID := "test-imported-de-1234" - containerName := "devsy-delete-test-imported-container" - volumeName := "devsy-agent-" + workspaceID - - _ = exec.CommandContext(ctx, "docker", "rm", "-f", containerName).Run() - _ = exec.CommandContext(ctx, "docker", "volume", "rm", "-f", volumeName).Run() - - out, err := exec.CommandContext(ctx, "docker", "run", "-d", - "--name", containerName, - "alpine:latest", "sleep", "300", - ).CombinedOutput() - require.NoError(t, err, string(out)) - t.Cleanup(func() { - _ = exec.CommandContext(context.Background(), "docker", "rm", "-f", containerName).Run() - _ = exec.CommandContext(context.Background(), "docker", "volume", "rm", "-f", volumeName).Run() - }) - - containerID := strings.TrimSpace(string(out)) - - out, err = exec.CommandContext(ctx, "docker", "volume", "create", - "--label", pkgconfig.DockerManagedLabel+"="+pkgconfig.LabelValueTrue, - "--label", pkgconfig.DockerResourceLabel+"="+pkgconfig.ResourceVolume, - "--label", pkgconfig.DockerWorkspaceIDLabel+"="+workspaceID, - "--label", pkgconfig.DockerVolumeRoleLabel+"="+pkgconfig.VolumeRoleAgent, - volumeName, - ).CombinedOutput() - require.NoError(t, err, string(out)) - - workspaceInfo := &provider2.AgentWorkspaceInfo{ - Workspace: &provider2.Workspace{ - ID: workspaceID, - Source: provider2.WorkspaceSource{Container: containerID}, - }, - } - - err = removeContainer(ctx, workspaceInfo, false) - require.NoError(t, err) - - out, _ = exec.CommandContext(ctx, "docker", "inspect", containerID).CombinedOutput() - require.NotContains(t, string(out), "No such object", - "imported container must not be deleted by devsy") - - out, _ = exec.CommandContext(ctx, "docker", "volume", "inspect", volumeName).CombinedOutput() - require.Contains(t, string(out), "No such volume", - "devsy-managed agent volume must be removed even for imported containers") -} -``` - -- [ ] **Step 2: Run test to verify it fails** - -Run: `go test -tags=integration ./cmd/internal/agentworkspace/... -run TestRemoveContainer_ImportedContainer_StillRemovesAgentVolume -v` -Expected: FAIL — the agent volume is still present because `removeContainer` never calls `runner.Delete()` for imported containers. - -- [ ] **Step 3: Commit the failing test** - -```bash -git add cmd/internal/agentworkspace/delete_integration_test.go -git commit -m "test(agentworkspace): reproduce agent volume leak on imported-container delete" ``` ---- - -### 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** +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 -[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) - } -} + 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())) ``` -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 3: Run test to verify it fails** -- [ ] **Step 2: 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. -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** +- [ ] **Step 4: Commit the failing test** ```bash -git add pkg/devcontainer/delete_test.go -git commit -m "test(devcontainer): reproduce silent delivery-volume cleanup failures" +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 4: Fix — `docker rm -v` to remove anonymous volumes +### 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 test, `pkg/driver/docker/lifecycle.go:210`, `pkg/workspace/rename.go:181`, and `e2e/tests/snapshot/helper.go` (all existing callers, no signature change). +- 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** @@ -294,9 +186,9 @@ func (r *DockerHelper) Remove(ctx context.Context, id string) error { } ``` -- [ ] **Step 2: Run Task 1's test to verify it now passes** +- [ ] **Step 2: Run Task 1's e2e test to verify it now passes** -Run: `go test -tags=integration ./pkg/docker/... -run TestDockerHelper_Remove_RemovesAnonymousVolumes -v` +Run the same command as Task 1 Step 3. Expected: PASS - [ ] **Step 3: Run the package's existing unit tests** @@ -313,95 +205,65 @@ git commit -m "fix(docker): remove anonymous volumes when deleting a container" --- -### Task 5: Fix — imported-container delete still cleans up the agent volume +### Task 3: Regression test — cleanup failures are invisible **Files:** -- Modify: `pkg/devcontainer/run.go:36-38` -- Modify: `pkg/devcontainer/delete.go:12-28` -- Modify: `cmd/internal/agentworkspace/delete.go:121-145` +- Modify: `pkg/devcontainer/delete_test.go` (append after line 211) **Interfaces:** -- Consumes: nothing new. -- Produces: `DeleteOptions.SkipContainerDelete bool` consumed by `runner.Delete` and by every caller constructing `devcontainer.DeleteOptions`. +- 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: Add the option** +- [ ] **Step 1: Write the failing test** ```go -[pkg/devcontainer/run.go PUT 36.=38:] -type DeleteOptions struct { - RemoveVolumes bool - SkipContainerDelete bool -} -``` - -- [ ] **Step 2: Honor the option in `Delete`** +[PUT >211:] +func TestCleanupDeliveryVolume_LogsWarningOnFailure(t *testing.T) { + logs := log.InitTestObserved(t, zapcore.WarnLevel) -```go -[pkg/devcontainer/delete.go PUT 12.=21:] -func (r *runner) Delete(ctx context.Context, options DeleteOptions) error { - containerDetails, err := r.driver.FindDevContainer(ctx, r.id) - if err != nil { - return fmt.Errorf("find dev container: %w", err) - } - defer r.cleanupDeliveryVolume(ctx) - defer r.cleanupImportedDevContainer() - if containerDetails == nil || options.SkipContainerDelete { - return nil + r := newTestRunner(&mockDriver{}) + r.workspaceConfig.Agent.Driver = provider.DockerDriver + r.workspaceConfig.Agent.Docker = provider.ProviderDockerDriverConfig{ + Path: "devsy-test-nonexistent-docker-binary", } -``` -- [ ] **Step 3: Always call `runner.Delete`, passing `SkipContainerDelete` instead of skipping the call** + r.cleanupDeliveryVolume(context.Background()) -```go -[cmd/internal/agentworkspace/delete.go PUT 121.=145:] -func removeContainer( - ctx context.Context, - workspaceInfo *provider2.AgentWorkspaceInfo, - removeVolumes bool, -) error { - log.Debugf("removing Devsy container from server: workspaceId=%s", workspaceInfo.Workspace.ID) - runner, err := CreateRunner(ctx, workspaceInfo) - if err != nil { - return err + if logs.Len() != 1 { + t.Fatalf("expected 1 warning log, got %d", logs.Len()) } - - imported := workspaceInfo.Workspace.Source.Container != "" - if imported { - log.Info("skipping container deletion, since it was not created by Devsy") + entry := logs.All()[0] + if entry.Level != zapcore.WarnLevel { + t.Errorf("expected warn level, got %v", entry.Level) } - - if err := runner.Delete(ctx, devcontainer.DeleteOptions{ - RemoveVolumes: removeVolumes, - SkipContainerDelete: imported, - }); err != nil { - return err + if !searchString(entry.Message, "delivery volume cleanup") { + t.Errorf("expected message to mention delivery volume cleanup, got: %s", entry.Message) } - log.Debug("removed Devsy container from server") - - return nil } ``` -- [ ] **Step 4: Run Task 2's test to verify it now passes** +Add to the import block at the top of `pkg/devcontainer/delete_test.go`: -Run: `go test -tags=integration ./cmd/internal/agentworkspace/... -run TestRemoveContainer_ImportedContainer_StillRemovesAgentVolume -v` -Expected: PASS +```go + "github.com/devsy-org/devsy/pkg/log" + "go.uber.org/zap/zapcore" +``` -- [ ] **Step 5: Run existing devcontainer and agentworkspace unit tests** +- [ ] **Step 2: Run test to verify it fails** -Run: `go test ./pkg/devcontainer/... ./cmd/internal/agentworkspace/...` -Expected: PASS, including the pre-existing `TestDelete_NilContainer_ReturnsNil`, `TestDelete_RunningContainer_StopsDeletesAndCleansUp`, etc. — none construct `DeleteOptions` with `SkipContainerDelete`, so the zero-value `false` preserves their behavior. +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 6: Commit** +- [ ] **Step 3: Commit the failing test** ```bash -git add pkg/devcontainer/run.go pkg/devcontainer/delete.go cmd/internal/agentworkspace/delete.go -git commit -m "fix(devcontainer): clean up agent volume for imported-container workspaces" +git add pkg/devcontainer/delete_test.go +git commit -m "test(devcontainer): reproduce silent delivery-volume cleanup failures" ``` --- -### Task 6: Fix — surface delivery-volume cleanup failures +### Task 4: Fix — surface delivery-volume cleanup failures **Files:** - Modify: `pkg/devcontainer/delete.go:43-47` @@ -440,21 +302,21 @@ git commit -m "fix(devcontainer): warn instead of silently swallowing delivery c --- -### Task 7: Verify and open draft PR +### Task 5: Verify and open draft PR **Files:** none (verification + PR only) **Interfaces:** none -- [ ] **Step 1: Run every touched package's tests together, including integration tags** +- [ ] **Step 1: Run every touched package's unit tests plus the new e2e spec** -Run: `go test ./pkg/docker/... ./pkg/devcontainer/... ./cmd/internal/agentworkspace/...` -Run: `go test -tags=integration ./pkg/docker/... ./cmd/internal/agentworkspace/... -run 'TestDockerHelper_Remove_RemovesAnonymousVolumes|TestRemoveContainer_ImportedContainer_StillRemovesAgentVolume' -v` +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/... ./cmd/internal/agentworkspace/...` +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** @@ -462,27 +324,26 @@ Expected: no errors ```bash git push -u origin HEAD gh pr create --draft \ - --title "fix: close three workspace-delete volume leaks" \ + --title "fix: close two workspace-delete volume leaks" \ --body "$(cat <<'EOF' ## Problem -`devsy workspace delete` leaks docker volumes in three independent ways: +`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. `removeContainer` skips the entire `runner.Delete()` call for workspaces attached to an imported/pre-existing container (`Source.Container != ""`), so the devsy-managed `devsy-agent-` volume is never cleaned up for those workspaces. -3. `cleanupDeliveryVolume` logs cleanup failures at Debug level, so a failed volume removal is indistinguishable from success unless `--debug` was already on. +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. `DeleteOptions.SkipContainerDelete` lets `removeContainer` keep skipping *container* removal for imported containers while still running the deferred volume cleanup inside `runner.Delete`. -3. Cleanup failures now log at Warn. +2. Cleanup failures now log at Warn. ## Tests -Each fix has a dedicated integration/unit test written first against the pre-fix code to confirm it reproduces the leak, then passing after the fix: -- `pkg/docker/helper_integration_test.go` -- `cmd/internal/agentworkspace/delete_integration_test.go` +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 )" @@ -494,8 +355,8 @@ Expected: draft PR URL printed. ## Self-Review -**Spec coverage:** all three findings from the investigation (anonymous-volume leak, imported-container skip, silent cleanup failure) each have a dedicated red-then-green test task and a dedicated fix task. No finding is left uncovered. +**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; every code block is complete and directly derived from the read source lines cited. +**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:** `DeleteOptions.SkipContainerDelete` is introduced once (Task 5, Step 1) and consumed with the same field name in `pkg/devcontainer/delete.go` and `cmd/internal/agentworkspace/delete.go`. `removeContainer`'s signature (`ctx, *provider2.AgentWorkspaceInfo, bool`) is unchanged between Task 2's test and Task 5's fix, so the test needs no follow-up edits. +**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). From fc57090e621a5d3d7d44eaf838e88f2febdde2c5 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Fri, 21 Aug 2026 05:43:29 +0000 Subject: [PATCH 3/7] test(e2e): reproduce anonymous volume leak on workspace delete --- e2e/tests/down/down.go | 47 +++++++++++++++++++ .../docker-anon-volume/.devcontainer.json | 6 +++ .../testdata/docker-anon-volume/Dockerfile | 2 + 3 files changed, 55 insertions(+) create mode 100644 e2e/tests/down/testdata/docker-anon-volume/.devcontainer.json create mode 100644 e2e/tests/down/testdata/docker-anon-volume/Dockerfile 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 From e60a6f58184b4167fbaded9332d18efb3fe05722 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Fri, 21 Aug 2026 05:47:23 +0000 Subject: [PATCH 4/7] fix(docker): remove anonymous volumes when deleting a container --- pkg/docker/helper.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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) } From 2ac1f36dd876eeb4f8938c3cb131e551c39b725f Mon Sep 17 00:00:00 2001 From: Samuel K Date: Fri, 21 Aug 2026 05:50:38 +0000 Subject: [PATCH 5/7] test(devcontainer): reproduce silent delivery-volume cleanup failures --- pkg/devcontainer/delete_test.go | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/pkg/devcontainer/delete_test.go b/pkg/devcontainer/delete_test.go index 21229eae6..43fef95eb 100644 --- a/pkg/devcontainer/delete_test.go +++ b/pkg/devcontainer/delete_test.go @@ -10,6 +10,8 @@ import ( "github.com/devsy-org/devsy/pkg/devcontainer/config" "github.com/devsy-org/devsy/pkg/driver" "github.com/devsy-org/devsy/pkg/provider" + "github.com/devsy-org/devsy/pkg/log" + "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) + } +} From f7f184a8e18931287e26c290cae8dc4888cc4629 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Fri, 21 Aug 2026 05:58:34 +0000 Subject: [PATCH 6/7] style(devcontainer): fix import order in delete_test.go (gofmt) --- pkg/devcontainer/delete_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/devcontainer/delete_test.go b/pkg/devcontainer/delete_test.go index 43fef95eb..ac17b856a 100644 --- a/pkg/devcontainer/delete_test.go +++ b/pkg/devcontainer/delete_test.go @@ -9,8 +9,8 @@ import ( "github.com/devsy-org/devsy/pkg/devcontainer/config" "github.com/devsy-org/devsy/pkg/driver" - "github.com/devsy-org/devsy/pkg/provider" "github.com/devsy-org/devsy/pkg/log" + "github.com/devsy-org/devsy/pkg/provider" "go.uber.org/zap/zapcore" ) From 561b49d36b6481064463788012ab0e6e8ddac8f5 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Fri, 21 Aug 2026 05:59:33 +0000 Subject: [PATCH 7/7] fix(devcontainer): warn instead of silently swallowing delivery cleanup failures --- pkg/devcontainer/delete.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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) } }