fix(delivery): clean up agent delivery volumes on workspace deletion by skevetter · Pull Request #266 · devsy-org/devsy

skevetter · 2026-05-12T12:40:35Z

Summary

Add best-effort volume cleanup to workspace Delete() path via cleanupDeliveryVolume()
Uses defer to ensure cleanup runs even when the container is already absent
deleteForRecreate() inherits cleanup through its existing Delete() call
6 unit tests covering all deletion paths and error safety

Summary by CodeRabbit

Release Notes

New Features
- New configuration option to specify a helper container image for local Docker delivery.
- Added automatic fallback mechanism: if helper container approach fails, the system seamlessly switches to direct file copy method.
- Improved cleanup: agent delivery volumes are now properly cleaned up during container deletion operations.

netlify · 2026-05-12T12:40:40Z

✅ Deploy Preview for devsydev canceled.

Name	Link
🔨 Latest commit	`9133d8d`
🔍 Latest deploy log	https://app.netlify.com/projects/devsydev/deploys/6a03a14256011d0008c8b87d

coderabbitai · 2026-05-12T12:40:44Z

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: c8e74602-ef7c-4282-acb4-d90dfba1a090

📥 Commits

Reviewing files that changed from the base of the PR and between 9d5a5b8 and 9133d8d.

📒 Files selected for processing (8)

pkg/agent/delivery/factory.go
pkg/agent/delivery/factory_test.go
pkg/agent/delivery/local_docker.go
pkg/agent/delivery/local_docker_test.go
pkg/devcontainer/delete.go
pkg/devcontainer/delete_test.go
pkg/devcontainer/setup.go
pkg/provider/provider.go

📝 Walkthrough

Walkthrough

This PR adds configurable helper image support to the Docker-based agent delivery system. A new HelperImage field flows from provider configuration through the factory into LocalDockerDelivery, which now buffers binary data and attempts population via a helper container before falling back to direct-copy mounting.

Changes

Helper Image Configuration and Delivery

Layer / File(s)	Summary
Configuration Model and Factory Wiring `pkg/provider/provider.go`, `pkg/agent/delivery/factory.go`, `pkg/agent/delivery/factory_test.go`	`ProviderDockerDriverConfig` and `FactoryOptions` gain `HelperImage` fields; `NewAgentDelivery` wires it into `LocalDockerDelivery`. Factory test updates `DockerCommand` to use constant instead of literal string.
LocalDockerDelivery Helper Image Implementation `pkg/agent/delivery/local_docker.go`	`LocalDockerDelivery` adds `HelperImage` field with `helperImageName()` selector defaulting to `busybox:latest`. `populateVolume` refactored to buffer binary once, try helper-based population, and fallback to direct-copy via new `populateVolumeDirectCopy` and `volumeMountpoint` helpers that write binary and set executable permissions.
LocalDockerDelivery Tests `pkg/agent/delivery/local_docker_test.go`	Tests verify `helperImageName()` defaults, configured override, HelperImage propagation through factory, and fallback to direct-copy path with binary executable validation.
Setup Integration and Devcontainer Cleanup `pkg/devcontainer/setup.go`, `pkg/devcontainer/delete.go`	`newAgentDelivery()` integrates `HelperImage` from `WorkspaceConfig`; `Delete()` defers volume cleanup and adds `cleanupDeliveryVolume()` helper for best-effort cleanup.
Devcontainer Delete Tests `pkg/devcontainer/delete_test.go`	Comprehensive test suite with `mockDriver` and `newTestRunner` helper, covering nil container, find errors, running/stopped lifecycle, delete errors, and cleanup assertions.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~22 minutes

Possibly related PRs

devsy-org/devsy#265: Extends AgentDelivery implementation by adding configurable HelperImage field and helper logic to LocalDockerDelivery, threading it through FactoryOptions and NewAgentDelivery.

Suggested labels

size/xl

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

…elivery Introduces a pluggable delivery strategy that replaces the monolithic inject.sh handshake with phase-aware delivery methods, eliminating race conditions identified in DEVSY-082. Three implementations: - LocalDockerDelivery: named volume pre-placement before container start - RemoteDockerDelivery: docker cp post-start for remote daemons - LegacyShellDelivery: wraps existing inject.sh path for custom drivers Includes factory, integration into setup.go/single.go, and E2E tests.

The existing up-* tests already exercise the delivery path through setupContainer → injectAgentIntoContainer. No need for separate tests.

Delete() now calls cleanupDeliveryVolume() after container removal, which invokes the delivery strategy's Cleanup() method to remove orphaned devsy-agent-{workspaceID} volumes. Cleanup is best-effort: errors are logged but never fail the delete operation. deleteForRecreate() inherits this fix via its existing call to Delete().

When a container is already absent (externally deleted or crashed), Delete() returned early before reaching the cleanup call. Using defer ensures cleanupDeliveryVolume() runs regardless of whether the container exists, preventing orphaned volumes in that scenario.

…oconst lint Resolves golangci-lint goconst violations in delete_test.go.

HelperImage field in ProviderDockerDriverConfig is threaded through FactoryOptions to LocalDockerDelivery. populateVolume() tries the configured helper container first (defaulting to busybox:latest), then falls back to writing the binary directly to the Docker volume mountpoint on the local filesystem. The direct-copy fallback is safe because LocalDockerDelivery is only used when Docker is local.

Fixes consumed io.Reader bug: binary data is now read once into a byte slice, then a fresh bytes.NewReader is passed to the helper container attempt. populateVolumeDirectCopy accepts []byte directly. Adds behavioral test for the fallback path using a fake docker script that fails on 'run' but returns a temp dir on 'volume inspect', verifying the binary lands with correct content and 0755 permissions.

… delivery tests

github-actions Bot added the size/xl label May 12, 2026

skevetter added 9 commits May 12, 2026 16:52

ci: add delivery label to PR integration test matrix

c39204c

refactor(e2e): remove redundant delivery E2E tests

7bdd166

The existing up-* tests already exercise the delivery path through setupContainer → injectAgentIntoContainer. No need for separate tests.

fix(test): extract repeated string literals as constants to satisfy g…

e5258fc

…oconst lint Resolves golangci-lint goconst violations in delete_test.go.

fix(test): use defaultDockerCmd constant instead of string literal in…

9133d8d

… delivery tests

skevetter force-pushed the f5d9-cf0b-ws-a-local-docker branch from c17c38b to 9133d8d Compare May 12, 2026 21:53

github-actions Bot added size/l and removed size/xl labels May 12, 2026

skevetter marked this pull request as ready for review May 12, 2026 23:00

skevetter merged commit c75008c into main May 12, 2026
95 of 100 checks passed

skevetter deleted the f5d9-cf0b-ws-a-local-docker branch May 12, 2026 23:01

coderabbitai Bot added the size/xl label May 12, 2026

coderabbitai Bot mentioned this pull request May 13, 2026

chore(delivery): add deprecation notices to LegacyShellDelivery #268

Merged

This was referenced May 16, 2026

chore(main): release 1.1.0 #302

Merged

chore(main): release 1.4.0-rc.1 #373

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(delivery): clean up agent delivery volumes on workspace deletion#266

fix(delivery): clean up agent delivery volumes on workspace deletion#266
skevetter merged 9 commits into
mainfrom
f5d9-cf0b-ws-a-local-docker

skevetter commented May 12, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

netlify Bot commented May 12, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented May 12, 2026 •

edited

Loading

Review failed

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Suggested labels

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

skevetter commented May 12, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Summary by CodeRabbit

Release Notes

Uh oh!

netlify Bot commented May 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for devsydev canceled.

Uh oh!

coderabbitai Bot commented May 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review failed

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Suggested labels

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

skevetter commented May 12, 2026 •

edited by coderabbitai Bot

Loading

netlify Bot commented May 12, 2026 •

edited

Loading

coderabbitai Bot commented May 12, 2026 •

edited

Loading