From 5fd2b8831a669b929f683a2e50134734141eea72 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Tue, 12 May 2026 02:34:17 -0500 Subject: [PATCH 1/9] feat(agent): implement AgentDelivery interface for container binary delivery 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. --- e2e/e2e_suite_test.go | 1 + e2e/tests/delivery/delivery.go | 112 +++++++++++++++++++++++++++++++++ 2 files changed, 113 insertions(+) create mode 100644 e2e/tests/delivery/delivery.go diff --git a/e2e/e2e_suite_test.go b/e2e/e2e_suite_test.go index 8333ab49b..058e3fa72 100644 --- a/e2e/e2e_suite_test.go +++ b/e2e/e2e_suite_test.go @@ -10,6 +10,7 @@ import ( // Register tests. _ "github.com/devsy-org/devsy/e2e/tests/build" _ "github.com/devsy-org/devsy/e2e/tests/context" + _ "github.com/devsy-org/devsy/e2e/tests/delivery" _ "github.com/devsy-org/devsy/e2e/tests/dockerinstall" _ "github.com/devsy-org/devsy/e2e/tests/down" _ "github.com/devsy-org/devsy/e2e/tests/exec" diff --git a/e2e/tests/delivery/delivery.go b/e2e/tests/delivery/delivery.go new file mode 100644 index 000000000..9b55c2065 --- /dev/null +++ b/e2e/tests/delivery/delivery.go @@ -0,0 +1,112 @@ +package delivery + +import ( + "context" + "os" + "os/exec" + "path/filepath" + + "github.com/devsy-org/devsy/e2e/framework" + "github.com/devsy-org/devsy/pkg/agent/delivery" + "github.com/devsy-org/devsy/pkg/devcontainer/config" + "github.com/devsy-org/devsy/pkg/driver" + "github.com/onsi/ginkgo/v2" + "github.com/onsi/gomega" +) + +var _ = ginkgo.Describe("agent delivery", ginkgo.Label("delivery"), func() { + ginkgo.Context("LocalDockerDelivery", func() { + ginkgo.It("should create volume, populate binary, and mount into RunOptions", + ginkgo.SpecTimeout(framework.TimeoutShort()), + func(ctx context.Context) { + d := &delivery.LocalDockerDelivery{DockerCommand: "docker"} + workspaceID := "e2e-delivery-local" + + runOpts := &driver.RunOptions{ + Mounts: []*config.Mount{}, + Env: map[string]string{}, + } + + binaryPath := findTestBinary() + + err := d.DeliverPreStart(ctx, delivery.PreStartOptions{ + WorkspaceID: workspaceID, + RunOptions: runOpts, + BinaryPath: binaryPath, + Arch: "amd64", + }) + framework.ExpectNoError(err) + ginkgo.DeferCleanup(func() { + _ = d.Cleanup(context.Background(), workspaceID) + }) + + gomega.Expect(runOpts.Mounts).To(gomega.HaveLen(1)) + expectedVolume := "devsy-agent-" + workspaceID + gomega.Expect(runOpts.Mounts[0].Source).To(gomega.Equal(expectedVolume)) + gomega.Expect(runOpts.Mounts[0].Type).To(gomega.Equal("volume")) + + // Verify binary in volume via docker run + out, err := exec.CommandContext(ctx, "docker", "run", "--rm", + "-v", "devsy-agent-"+workspaceID+":/opt/devsy", + "busybox:latest", "test", "-x", "/opt/devsy/devsy", + ).CombinedOutput() + framework.ExpectNoError( + err, "binary should be executable in volume: %s", string(out), + ) + }) + }) + + ginkgo.Context("RemoteDockerDelivery", func() { + ginkgo.It("should copy binary into running container via docker cp", + ginkgo.SpecTimeout(framework.TimeoutShort()), + func(ctx context.Context) { + containerName := "e2e-delivery-remote" + + out, err := exec.CommandContext(ctx, "docker", "run", "-d", + "--name", containerName, + "busybox:latest", "sleep", "120", + ).CombinedOutput() + framework.ExpectNoError(err, "failed to start test container: %s", string(out)) + ginkgo.DeferCleanup(func() { + _ = exec.Command("docker", "rm", "-f", containerName).Run() + }) + + d := &delivery.RemoteDockerDelivery{ + DockerCommand: "docker", + ContainerID: containerName, + } + + binaryPath := findTestBinary() + + err = d.DeliverPostStart(ctx, delivery.PostStartOptions{ + WorkspaceID: "e2e-workspace", + BinaryPath: binaryPath, + Arch: "amd64", + }) + framework.ExpectNoError(err) + + // Verify binary exists and is executable + out, err = exec.CommandContext(ctx, "docker", "exec", containerName, + "test", "-x", "/usr/local/bin/devsy", + ).CombinedOutput() + framework.ExpectNoError(err, "binary should be executable: %s", string(out)) + }) + }) +}) + +func findTestBinary() string { + candidates := []string{"/bin/sh", "/bin/busybox"} + for _, c := range candidates { + if _, err := os.Stat(c); err == nil { + return c + } + } + + binDir, _ := os.Getwd() + devsy := filepath.Join(binDir, "bin", "devsy-linux-amd64") + if _, err := os.Stat(devsy); err == nil { + return devsy + } + + return "/bin/sh" +} From c39204c1839c26d33a4a142b8f0420e1d640a9bc Mon Sep 17 00:00:00 2001 From: Samuel K Date: Tue, 12 May 2026 02:38:59 -0500 Subject: [PATCH 2/9] ci: add delivery label to PR integration test matrix --- .github/workflows/pr-ci.yml | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/.github/workflows/pr-ci.yml b/.github/workflows/pr-ci.yml index 0a575c2bc..289486d14 100644 --- a/.github/workflows/pr-ci.yml +++ b/.github/workflows/pr-ci.yml @@ -228,6 +228,12 @@ jobs: install-kind: false requires-secret: false + - label: delivery + runner: ubuntu-latest + free-disk-space: false + install-kind: false + requires-secret: false + # Up tests - label: up-workspaces From 7bdd166879015c2799de9974f61409d99faad11f Mon Sep 17 00:00:00 2001 From: Samuel K Date: Tue, 12 May 2026 02:44:22 -0500 Subject: [PATCH 3/9] refactor(e2e): remove redundant delivery E2E tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The existing up-* tests already exercise the delivery path through setupContainer → injectAgentIntoContainer. No need for separate tests. --- .github/workflows/pr-ci.yml | 6 -- e2e/e2e_suite_test.go | 1 - e2e/tests/delivery/delivery.go | 112 --------------------------------- 3 files changed, 119 deletions(-) delete mode 100644 e2e/tests/delivery/delivery.go diff --git a/.github/workflows/pr-ci.yml b/.github/workflows/pr-ci.yml index 289486d14..0a575c2bc 100644 --- a/.github/workflows/pr-ci.yml +++ b/.github/workflows/pr-ci.yml @@ -228,12 +228,6 @@ jobs: install-kind: false requires-secret: false - - label: delivery - runner: ubuntu-latest - free-disk-space: false - install-kind: false - requires-secret: false - # Up tests - label: up-workspaces diff --git a/e2e/e2e_suite_test.go b/e2e/e2e_suite_test.go index 058e3fa72..8333ab49b 100644 --- a/e2e/e2e_suite_test.go +++ b/e2e/e2e_suite_test.go @@ -10,7 +10,6 @@ import ( // Register tests. _ "github.com/devsy-org/devsy/e2e/tests/build" _ "github.com/devsy-org/devsy/e2e/tests/context" - _ "github.com/devsy-org/devsy/e2e/tests/delivery" _ "github.com/devsy-org/devsy/e2e/tests/dockerinstall" _ "github.com/devsy-org/devsy/e2e/tests/down" _ "github.com/devsy-org/devsy/e2e/tests/exec" diff --git a/e2e/tests/delivery/delivery.go b/e2e/tests/delivery/delivery.go deleted file mode 100644 index 9b55c2065..000000000 --- a/e2e/tests/delivery/delivery.go +++ /dev/null @@ -1,112 +0,0 @@ -package delivery - -import ( - "context" - "os" - "os/exec" - "path/filepath" - - "github.com/devsy-org/devsy/e2e/framework" - "github.com/devsy-org/devsy/pkg/agent/delivery" - "github.com/devsy-org/devsy/pkg/devcontainer/config" - "github.com/devsy-org/devsy/pkg/driver" - "github.com/onsi/ginkgo/v2" - "github.com/onsi/gomega" -) - -var _ = ginkgo.Describe("agent delivery", ginkgo.Label("delivery"), func() { - ginkgo.Context("LocalDockerDelivery", func() { - ginkgo.It("should create volume, populate binary, and mount into RunOptions", - ginkgo.SpecTimeout(framework.TimeoutShort()), - func(ctx context.Context) { - d := &delivery.LocalDockerDelivery{DockerCommand: "docker"} - workspaceID := "e2e-delivery-local" - - runOpts := &driver.RunOptions{ - Mounts: []*config.Mount{}, - Env: map[string]string{}, - } - - binaryPath := findTestBinary() - - err := d.DeliverPreStart(ctx, delivery.PreStartOptions{ - WorkspaceID: workspaceID, - RunOptions: runOpts, - BinaryPath: binaryPath, - Arch: "amd64", - }) - framework.ExpectNoError(err) - ginkgo.DeferCleanup(func() { - _ = d.Cleanup(context.Background(), workspaceID) - }) - - gomega.Expect(runOpts.Mounts).To(gomega.HaveLen(1)) - expectedVolume := "devsy-agent-" + workspaceID - gomega.Expect(runOpts.Mounts[0].Source).To(gomega.Equal(expectedVolume)) - gomega.Expect(runOpts.Mounts[0].Type).To(gomega.Equal("volume")) - - // Verify binary in volume via docker run - out, err := exec.CommandContext(ctx, "docker", "run", "--rm", - "-v", "devsy-agent-"+workspaceID+":/opt/devsy", - "busybox:latest", "test", "-x", "/opt/devsy/devsy", - ).CombinedOutput() - framework.ExpectNoError( - err, "binary should be executable in volume: %s", string(out), - ) - }) - }) - - ginkgo.Context("RemoteDockerDelivery", func() { - ginkgo.It("should copy binary into running container via docker cp", - ginkgo.SpecTimeout(framework.TimeoutShort()), - func(ctx context.Context) { - containerName := "e2e-delivery-remote" - - out, err := exec.CommandContext(ctx, "docker", "run", "-d", - "--name", containerName, - "busybox:latest", "sleep", "120", - ).CombinedOutput() - framework.ExpectNoError(err, "failed to start test container: %s", string(out)) - ginkgo.DeferCleanup(func() { - _ = exec.Command("docker", "rm", "-f", containerName).Run() - }) - - d := &delivery.RemoteDockerDelivery{ - DockerCommand: "docker", - ContainerID: containerName, - } - - binaryPath := findTestBinary() - - err = d.DeliverPostStart(ctx, delivery.PostStartOptions{ - WorkspaceID: "e2e-workspace", - BinaryPath: binaryPath, - Arch: "amd64", - }) - framework.ExpectNoError(err) - - // Verify binary exists and is executable - out, err = exec.CommandContext(ctx, "docker", "exec", containerName, - "test", "-x", "/usr/local/bin/devsy", - ).CombinedOutput() - framework.ExpectNoError(err, "binary should be executable: %s", string(out)) - }) - }) -}) - -func findTestBinary() string { - candidates := []string{"/bin/sh", "/bin/busybox"} - for _, c := range candidates { - if _, err := os.Stat(c); err == nil { - return c - } - } - - binDir, _ := os.Getwd() - devsy := filepath.Join(binDir, "bin", "devsy-linux-amd64") - if _, err := os.Stat(devsy); err == nil { - return devsy - } - - return "/bin/sh" -} From eaed848840916bdc4757464d6e5cefb98fc29690 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Tue, 12 May 2026 07:18:06 -0500 Subject: [PATCH 4/9] fix(delivery): clean up agent delivery volumes on workspace deletion 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(). --- pkg/devcontainer/delete.go | 9 ++ pkg/devcontainer/delete_test.go | 174 ++++++++++++++++++++++++++++++++ 2 files changed, 183 insertions(+) create mode 100644 pkg/devcontainer/delete_test.go diff --git a/pkg/devcontainer/delete.go b/pkg/devcontainer/delete.go index cb3d95122..d568a7897 100644 --- a/pkg/devcontainer/delete.go +++ b/pkg/devcontainer/delete.go @@ -37,9 +37,18 @@ func (r *runner) Delete(ctx context.Context, options DeleteOptions) error { } } + r.cleanupDeliveryVolume(ctx) + return nil } +func (r *runner) cleanupDeliveryVolume(ctx context.Context) { + strategy := r.newAgentDelivery() + if err := strategy.Cleanup(ctx, r.ID); err != nil { + log.Debugf("best-effort agent delivery volume cleanup: %v", err) + } +} + func (r *runner) Stop(ctx context.Context) error { containerDetails, err := r.Driver.FindDevContainer(ctx, r.ID) if err != nil { diff --git a/pkg/devcontainer/delete_test.go b/pkg/devcontainer/delete_test.go new file mode 100644 index 000000000..47736afd1 --- /dev/null +++ b/pkg/devcontainer/delete_test.go @@ -0,0 +1,174 @@ +package devcontainer + +import ( + "context" + "fmt" + "io" + "testing" + + "github.com/devsy-org/devsy/pkg/devcontainer/config" + "github.com/devsy-org/devsy/pkg/driver" + "github.com/devsy-org/devsy/pkg/provider" +) + +type mockDriver struct { + findResult *config.ContainerDetails + findErr error + stopCalled bool + stopErr error + deleteCalled bool + deleteErr error +} + +func (m *mockDriver) FindDevContainer( + _ context.Context, + _ string, +) (*config.ContainerDetails, error) { + return m.findResult, m.findErr +} + +func (m *mockDriver) StopDevContainer(_ context.Context, _ string) error { + m.stopCalled = true + return m.stopErr +} + +func (m *mockDriver) DeleteDevContainer(_ context.Context, _ string) error { + m.deleteCalled = true + return m.deleteErr +} + +//nolint:revive // interface implementation requires 7 args +func (m *mockDriver) CommandDevContainer( + _ context.Context, _, _, _ string, _ io.Reader, _ io.Writer, _ io.Writer, +) error { + return nil +} + +func (m *mockDriver) RunDevContainer(_ context.Context, _ string, _ *driver.RunOptions) error { + return nil +} + +func (m *mockDriver) TargetArchitecture(_ context.Context, _ string) (string, error) { + return "amd64", nil +} + +func (m *mockDriver) StartDevContainer(_ context.Context, _ string) error { + return nil +} + +func (m *mockDriver) GetDevContainerLogs( + _ context.Context, _ string, _ io.Writer, _ io.Writer, +) error { + return nil +} + +func newTestRunner(d driver.Driver) *runner { + return &runner{ + Driver: d, + ID: "test-workspace", + WorkspaceConfig: &provider.AgentWorkspaceInfo{ + Agent: provider.ProviderAgentConfig{ + Driver: provider.CustomDriver, + }, + }, + } +} + +func TestDelete_NilContainer_ReturnsNil(t *testing.T) { + d := &mockDriver{findResult: nil} + r := newTestRunner(d) + + err := r.Delete(context.Background(), DeleteOptions{}) + if err != nil { + t.Fatalf("expected nil error, got: %v", err) + } + if d.stopCalled { + t.Error("StopDevContainer should not be called when container is nil") + } + if d.deleteCalled { + t.Error("DeleteDevContainer should not be called when container is nil") + } +} + +func TestDelete_FindError_ReturnsError(t *testing.T) { + d := &mockDriver{findErr: fmt.Errorf("connection refused")} + r := newTestRunner(d) + + err := r.Delete(context.Background(), DeleteOptions{}) + + if err == nil { + t.Fatal("expected error, got nil") + } + if !searchString(err.Error(), "find dev container") { + t.Errorf("expected wrapped find error, got: %v", err) + } +} + +func TestDelete_RunningContainer_StopsDeletesAndCleansUp(t *testing.T) { + d := &mockDriver{ + findResult: &config.ContainerDetails{ + ID: "container-abc", + State: config.ContainerDetailsState{Status: "running"}, + Config: config.ContainerDetailsConfig{Labels: map[string]string{}}, + }, + } + r := newTestRunner(d) + + err := r.Delete(context.Background(), DeleteOptions{}) + if err != nil { + t.Fatalf("expected nil error, got: %v", err) + } + if !d.stopCalled { + t.Error("expected StopDevContainer to be called for running container") + } + if !d.deleteCalled { + t.Error("expected DeleteDevContainer to be called") + } +} + +func TestDelete_StoppedContainer_SkipsStopAndDeletes(t *testing.T) { + d := &mockDriver{ + findResult: &config.ContainerDetails{ + ID: "container-abc", + State: config.ContainerDetailsState{Status: "exited"}, + Config: config.ContainerDetailsConfig{Labels: map[string]string{}}, + }, + } + r := newTestRunner(d) + + err := r.Delete(context.Background(), DeleteOptions{}) + if err != nil { + t.Fatalf("expected nil error, got: %v", err) + } + if d.stopCalled { + t.Error("StopDevContainer should not be called for stopped container") + } + if !d.deleteCalled { + t.Error("expected DeleteDevContainer to be called") + } +} + +func TestDelete_DeleteError_ReturnsError(t *testing.T) { + d := &mockDriver{ + findResult: &config.ContainerDetails{ + ID: "container-abc", + State: config.ContainerDetailsState{Status: "exited"}, + Config: config.ContainerDetailsConfig{Labels: map[string]string{}}, + }, + deleteErr: fmt.Errorf("permission denied"), + } + r := newTestRunner(d) + + err := r.Delete(context.Background(), DeleteOptions{}) + + if err == nil { + t.Fatal("expected error from DeleteDevContainer, got nil") + } +} + +func TestCleanupDeliveryVolume_DoesNotPanic(t *testing.T) { + d := &mockDriver{} + r := newTestRunner(d) + + r.cleanupDeliveryVolume(context.Background()) +} From 5f311af6b2f3427882bd494e1255bfcb40b58043 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Tue, 12 May 2026 07:39:50 -0500 Subject: [PATCH 5/9] fix(delivery): use defer for volume cleanup to cover nil-container path 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. --- pkg/devcontainer/delete.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/devcontainer/delete.go b/pkg/devcontainer/delete.go index d568a7897..e01a92c61 100644 --- a/pkg/devcontainer/delete.go +++ b/pkg/devcontainer/delete.go @@ -13,7 +13,9 @@ 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) - } else if containerDetails == nil { + } + defer r.cleanupDeliveryVolume(ctx) + if containerDetails == nil { return nil } @@ -37,8 +39,6 @@ func (r *runner) Delete(ctx context.Context, options DeleteOptions) error { } } - r.cleanupDeliveryVolume(ctx) - return nil } From e5258fc47be0edd3640156b4686ccb210fd9cdcb Mon Sep 17 00:00:00 2001 From: Samuel K Date: Tue, 12 May 2026 07:49:19 -0500 Subject: [PATCH 6/9] fix(test): extract repeated string literals as constants to satisfy goconst lint Resolves golangci-lint goconst violations in delete_test.go. --- pkg/devcontainer/delete_test.go | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/pkg/devcontainer/delete_test.go b/pkg/devcontainer/delete_test.go index 47736afd1..01387bd7c 100644 --- a/pkg/devcontainer/delete_test.go +++ b/pkg/devcontainer/delete_test.go @@ -11,6 +11,11 @@ import ( "github.com/devsy-org/devsy/pkg/provider" ) +const ( + testContainerID = "container-abc" + testStatusRunning = "running" +) + type mockDriver struct { findResult *config.ContainerDetails findErr error @@ -107,8 +112,8 @@ func TestDelete_FindError_ReturnsError(t *testing.T) { func TestDelete_RunningContainer_StopsDeletesAndCleansUp(t *testing.T) { d := &mockDriver{ findResult: &config.ContainerDetails{ - ID: "container-abc", - State: config.ContainerDetailsState{Status: "running"}, + ID: testContainerID, + State: config.ContainerDetailsState{Status: testStatusRunning}, Config: config.ContainerDetailsConfig{Labels: map[string]string{}}, }, } @@ -129,7 +134,7 @@ func TestDelete_RunningContainer_StopsDeletesAndCleansUp(t *testing.T) { func TestDelete_StoppedContainer_SkipsStopAndDeletes(t *testing.T) { d := &mockDriver{ findResult: &config.ContainerDetails{ - ID: "container-abc", + ID: testContainerID, State: config.ContainerDetailsState{Status: "exited"}, Config: config.ContainerDetailsConfig{Labels: map[string]string{}}, }, @@ -151,7 +156,7 @@ func TestDelete_StoppedContainer_SkipsStopAndDeletes(t *testing.T) { func TestDelete_DeleteError_ReturnsError(t *testing.T) { d := &mockDriver{ findResult: &config.ContainerDetails{ - ID: "container-abc", + ID: testContainerID, State: config.ContainerDetailsState{Status: "exited"}, Config: config.ContainerDetailsConfig{Labels: map[string]string{}}, }, From ad219fd1830b21cfbe80f2cbe272647b762b093e Mon Sep 17 00:00:00 2001 From: Samuel K Date: Tue, 12 May 2026 07:53:23 -0500 Subject: [PATCH 7/9] feat(delivery): make helper image configurable with direct-copy fallback 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. --- pkg/agent/delivery/factory.go | 2 + pkg/agent/delivery/local_docker.go | 77 +++++++++++++++++++++++-- pkg/agent/delivery/local_docker_test.go | 45 +++++++++++++++ pkg/devcontainer/setup.go | 1 + pkg/provider/provider.go | 4 ++ 5 files changed, 124 insertions(+), 5 deletions(-) diff --git a/pkg/agent/delivery/factory.go b/pkg/agent/delivery/factory.go index c42bde2d4..79804a013 100644 --- a/pkg/agent/delivery/factory.go +++ b/pkg/agent/delivery/factory.go @@ -16,6 +16,7 @@ type FactoryOptions struct { WorkspaceID string DockerCommand string DockerEnv []string + HelperImage string IsRemoteDocker bool ContainerID string ExecFunc inject.ExecFunc @@ -46,6 +47,7 @@ func NewAgentDelivery(opts FactoryOptions) AgentDelivery { return &LocalDockerDelivery{ DockerCommand: opts.DockerCommand, Environment: opts.DockerEnv, + HelperImage: opts.HelperImage, } } log.Debugf("using remote docker delivery for non-local docker daemon") diff --git a/pkg/agent/delivery/local_docker.go b/pkg/agent/delivery/local_docker.go index 90caf203d..58e16087a 100644 --- a/pkg/agent/delivery/local_docker.go +++ b/pkg/agent/delivery/local_docker.go @@ -3,8 +3,11 @@ package delivery import ( "context" "fmt" + "io" "os" "os/exec" + "path/filepath" + "strings" "github.com/devsy-org/devsy/pkg/agent" "github.com/devsy-org/devsy/pkg/devcontainer/config" @@ -14,15 +17,16 @@ import ( var _ AgentDelivery = (*LocalDockerDelivery)(nil) const ( - defaultDockerCmd = "docker" - volumePrefix = "devsy-agent-" - volumeMountPath = "/opt/devsy" - helperImage = "busybox:latest" + defaultDockerCmd = "docker" + volumePrefix = "devsy-agent-" + volumeMountPath = "/opt/devsy" + defaultHelperImage = "busybox:latest" ) type LocalDockerDelivery struct { DockerCommand string Environment []string + HelperImage string } func (d *LocalDockerDelivery) Phase() DeliveryPhase { @@ -77,6 +81,13 @@ func (d *LocalDockerDelivery) createVolume(ctx context.Context, name string) err return nil } +func (d *LocalDockerDelivery) helperImageName() string { + if d.HelperImage != "" { + return d.HelperImage + } + return defaultHelperImage +} + func (d *LocalDockerDelivery) populateVolume( ctx context.Context, volumeName string, @@ -89,6 +100,20 @@ func (d *LocalDockerDelivery) populateVolume( } defer func() { _ = binary.Close() }() + err = d.populateVolumeWithHelper(ctx, volumeName, binary) + if err == nil { + return nil + } + log.Debugf("helper container populate failed, trying direct copy: %v", err) + + return d.populateVolumeDirectCopy(ctx, volumeName, binary) +} + +func (d *LocalDockerDelivery) populateVolumeWithHelper( + ctx context.Context, + volumeName string, + binary io.Reader, +) error { containerName := "devsy-agent-init-" + volumeName script := fmt.Sprintf( "cat > %s/%s && chmod 755 %s/%s", @@ -99,7 +124,7 @@ func (d *LocalDockerDelivery) populateVolume( "--name", containerName, "-v", volumeName + ":" + volumeMountPath, "-i", - helperImage, + d.helperImageName(), "sh", "-c", script, } @@ -113,6 +138,48 @@ func (d *LocalDockerDelivery) populateVolume( return nil } +func (d *LocalDockerDelivery) populateVolumeDirectCopy( + ctx context.Context, + volumeName string, + binary io.Reader, +) error { + mountpoint, err := d.volumeMountpoint(ctx, volumeName) + if err != nil { + return fmt.Errorf("inspect volume mountpoint: %w", err) + } + + destPath := filepath.Join(mountpoint, binaryName()) + data, err := io.ReadAll(binary) + if err != nil { + return fmt.Errorf("read binary: %w", err) + } + + if err := os.WriteFile(destPath, data, 0o600); err != nil { + return fmt.Errorf("write binary to volume: %w", err) + } + // #nosec G302 -- agent binary must be executable + if err := os.Chmod(destPath, 0o755); err != nil { + return fmt.Errorf("chmod binary: %w", err) + } + + return nil +} + +func (d *LocalDockerDelivery) volumeMountpoint( + ctx context.Context, + volumeName string, +) (string, error) { + out, err := d.cmd( + ctx, "volume", "inspect", + "--format", "{{.Mountpoint}}", + volumeName, + ).CombinedOutput() + if err != nil { + return "", fmt.Errorf("%s: %w", string(out), err) + } + return strings.TrimSpace(string(out)), nil +} + func (d *LocalDockerDelivery) removeVolume(ctx context.Context, workspaceID string) error { volumeName := volumePrefix + workspaceID out, err := d.cmd(ctx, "volume", "rm", "-f", volumeName).CombinedOutput() diff --git a/pkg/agent/delivery/local_docker_test.go b/pkg/agent/delivery/local_docker_test.go index 0f94b4f19..9d4622f46 100644 --- a/pkg/agent/delivery/local_docker_test.go +++ b/pkg/agent/delivery/local_docker_test.go @@ -4,6 +4,7 @@ import ( "context" "testing" + "github.com/devsy-org/devsy/pkg/provider" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -37,3 +38,47 @@ func TestBinaryName(t *testing.T) { name := binaryName() assert.Equal(t, "devsy", name) } + +func TestLocalDockerDelivery_HelperImageName_Default(t *testing.T) { + d := &LocalDockerDelivery{} + assert.Equal(t, "busybox:latest", d.helperImageName()) +} + +func TestLocalDockerDelivery_HelperImageName_Configured(t *testing.T) { + d := &LocalDockerDelivery{HelperImage: "registry.internal/tools/busybox:1.36"} + assert.Equal(t, "registry.internal/tools/busybox:1.36", d.helperImageName()) +} + +func TestNewAgentDelivery_LocalDocker_ThreadsHelperImage(t *testing.T) { + opts := FactoryOptions{ + WorkspaceConfig: &provider.AgentWorkspaceInfo{ + Agent: provider.ProviderAgentConfig{ + Driver: provider.DockerDriver, + }, + }, + DockerCommand: "docker", + HelperImage: "my-registry/busybox:1.35", + } + + d := NewAgentDelivery(opts) + local, ok := d.(*LocalDockerDelivery) + require.True(t, ok) + assert.Equal(t, "my-registry/busybox:1.35", local.HelperImage) +} + +func TestNewAgentDelivery_LocalDocker_EmptyHelperImage(t *testing.T) { + opts := FactoryOptions{ + WorkspaceConfig: &provider.AgentWorkspaceInfo{ + Agent: provider.ProviderAgentConfig{ + Driver: provider.DockerDriver, + }, + }, + DockerCommand: "docker", + } + + d := NewAgentDelivery(opts) + local, ok := d.(*LocalDockerDelivery) + require.True(t, ok) + assert.Empty(t, local.HelperImage) + assert.Equal(t, "busybox:latest", local.helperImageName()) +} diff --git a/pkg/devcontainer/setup.go b/pkg/devcontainer/setup.go index 0a3ef8ae4..c2468332b 100644 --- a/pkg/devcontainer/setup.go +++ b/pkg/devcontainer/setup.go @@ -97,6 +97,7 @@ func (r *runner) newAgentDelivery() delivery.AgentDelivery { WorkspaceID: r.ID, DockerCommand: dockerCmd, DockerEnv: dockerEnv, + HelperImage: r.WorkspaceConfig.Agent.Docker.HelperImage, ContainerID: r.ID, ExecFunc: execFn, }) diff --git a/pkg/provider/provider.go b/pkg/provider/provider.go index 6e6e6eb3d..e153997ed 100644 --- a/pkg/provider/provider.go +++ b/pkg/provider/provider.go @@ -194,6 +194,10 @@ type ProviderDockerDriverConfig struct { // Environment variables to set when running docker commands Env map[string]string `json:"env,omitempty"` + + // HelperImage is used by LocalDockerDelivery for volume population. + // When empty, defaults to busybox:latest with automatic fallback to direct copy. + HelperImage string `json:"helperImage,omitempty"` } type ProviderKubernetesDriverConfig struct { From c78cd5bbf8c86e17bd2a905c1842e1857d7bd3c6 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Tue, 12 May 2026 08:01:47 -0500 Subject: [PATCH 8/9] fix(delivery): buffer binary before populate attempts, add fallback test 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. --- pkg/agent/delivery/local_docker.go | 16 +++++----- pkg/agent/delivery/local_docker_test.go | 42 +++++++++++++++++++++++++ pkg/provider/provider.go | 2 +- 3 files changed, 52 insertions(+), 8 deletions(-) diff --git a/pkg/agent/delivery/local_docker.go b/pkg/agent/delivery/local_docker.go index 58e16087a..e236b78b8 100644 --- a/pkg/agent/delivery/local_docker.go +++ b/pkg/agent/delivery/local_docker.go @@ -1,6 +1,7 @@ package delivery import ( + "bytes" "context" "fmt" "io" @@ -100,13 +101,18 @@ func (d *LocalDockerDelivery) populateVolume( } defer func() { _ = binary.Close() }() - err = d.populateVolumeWithHelper(ctx, volumeName, binary) + data, err := io.ReadAll(binary) + if err != nil { + return fmt.Errorf("read binary: %w", err) + } + + err = d.populateVolumeWithHelper(ctx, volumeName, bytes.NewReader(data)) if err == nil { return nil } log.Debugf("helper container populate failed, trying direct copy: %v", err) - return d.populateVolumeDirectCopy(ctx, volumeName, binary) + return d.populateVolumeDirectCopy(ctx, volumeName, data) } func (d *LocalDockerDelivery) populateVolumeWithHelper( @@ -141,7 +147,7 @@ func (d *LocalDockerDelivery) populateVolumeWithHelper( func (d *LocalDockerDelivery) populateVolumeDirectCopy( ctx context.Context, volumeName string, - binary io.Reader, + data []byte, ) error { mountpoint, err := d.volumeMountpoint(ctx, volumeName) if err != nil { @@ -149,10 +155,6 @@ func (d *LocalDockerDelivery) populateVolumeDirectCopy( } destPath := filepath.Join(mountpoint, binaryName()) - data, err := io.ReadAll(binary) - if err != nil { - return fmt.Errorf("read binary: %w", err) - } if err := os.WriteFile(destPath, data, 0o600); err != nil { return fmt.Errorf("write binary to volume: %w", err) diff --git a/pkg/agent/delivery/local_docker_test.go b/pkg/agent/delivery/local_docker_test.go index 9d4622f46..ce4f7af0c 100644 --- a/pkg/agent/delivery/local_docker_test.go +++ b/pkg/agent/delivery/local_docker_test.go @@ -1,7 +1,11 @@ package delivery import ( + "bytes" "context" + "io" + "os" + "path/filepath" "testing" "github.com/devsy-org/devsy/pkg/provider" @@ -82,3 +86,41 @@ func TestNewAgentDelivery_LocalDocker_EmptyHelperImage(t *testing.T) { assert.Empty(t, local.HelperImage) assert.Equal(t, "busybox:latest", local.helperImageName()) } + +func TestPopulateVolume_FallbackToDirectCopy(t *testing.T) { + tmpDir := t.TempDir() + mountDir := filepath.Join(tmpDir, "mount") + require.NoError(t, os.MkdirAll(mountDir, 0o750)) + + scriptPath := filepath.Join(tmpDir, "fake-docker.sh") + script := "#!/bin/sh\n" + + "case \"$1\" in\n" + + " run) echo \"image not found\" >&2; exit 1 ;;\n" + + " volume) echo \"" + mountDir + "\" ;;\n" + + " *) exit 1 ;;\n" + + "esac\n" + require.NoError(t, os.WriteFile(scriptPath, []byte(script), 0o600)) + // #nosec G302 -- test script must be executable + require.NoError(t, os.Chmod(scriptPath, 0o755)) + + binaryContent := []byte("fake-agent-binary-content") + binarySource := func(_ context.Context, _ string) (io.ReadCloser, error) { + return io.NopCloser(bytes.NewReader(binaryContent)), nil + } + + d := &LocalDockerDelivery{ + DockerCommand: scriptPath, + } + + err := d.populateVolume(context.Background(), "test-vol", binarySource, "amd64") + require.NoError(t, err) + + destPath := filepath.Join(mountDir, binaryName()) + data, err := os.ReadFile(destPath) //nolint:gosec // test reads from a temp directory we control + require.NoError(t, err) + assert.Equal(t, binaryContent, data) + + info, err := os.Stat(destPath) + require.NoError(t, err) + assert.Equal(t, os.FileMode(0o755), info.Mode().Perm()) +} diff --git a/pkg/provider/provider.go b/pkg/provider/provider.go index e153997ed..845333bae 100644 --- a/pkg/provider/provider.go +++ b/pkg/provider/provider.go @@ -196,7 +196,7 @@ type ProviderDockerDriverConfig struct { Env map[string]string `json:"env,omitempty"` // HelperImage is used by LocalDockerDelivery for volume population. - // When empty, defaults to busybox:latest with automatic fallback to direct copy. + // When empty, defaults to busybox:latest. A direct-copy fallback is used if the helper container approach fails. HelperImage string `json:"helperImage,omitempty"` } From 9133d8ddd25263d431ee923e18c2d6a704950710 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Tue, 12 May 2026 08:14:04 -0500 Subject: [PATCH 9/9] fix(test): use defaultDockerCmd constant instead of string literal in delivery tests --- pkg/agent/delivery/factory_test.go | 2 +- pkg/agent/delivery/local_docker_test.go | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/agent/delivery/factory_test.go b/pkg/agent/delivery/factory_test.go index 8f6f97fe9..a881189f1 100644 --- a/pkg/agent/delivery/factory_test.go +++ b/pkg/agent/delivery/factory_test.go @@ -17,7 +17,7 @@ func TestNewAgentDelivery_LocalDocker(t *testing.T) { Driver: provider.DockerDriver, }, }, - DockerCommand: "docker", + DockerCommand: defaultDockerCmd, } d := NewAgentDelivery(opts) diff --git a/pkg/agent/delivery/local_docker_test.go b/pkg/agent/delivery/local_docker_test.go index ce4f7af0c..6d9861707 100644 --- a/pkg/agent/delivery/local_docker_test.go +++ b/pkg/agent/delivery/local_docker_test.go @@ -60,7 +60,7 @@ func TestNewAgentDelivery_LocalDocker_ThreadsHelperImage(t *testing.T) { Driver: provider.DockerDriver, }, }, - DockerCommand: "docker", + DockerCommand: defaultDockerCmd, HelperImage: "my-registry/busybox:1.35", } @@ -77,7 +77,7 @@ func TestNewAgentDelivery_LocalDocker_EmptyHelperImage(t *testing.T) { Driver: provider.DockerDriver, }, }, - DockerCommand: "docker", + DockerCommand: defaultDockerCmd, } d := NewAgentDelivery(opts)