From 144e5ca3bbfbf5830dec7bc93eb817235fe50a35 Mon Sep 17 00:00:00 2001 From: bilby91 Date: Sat, 22 Aug 2026 21:18:08 -0300 Subject: [PATCH] fix(compose): build sidecar images and adopt foreign containers on the native path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two gaps versus the shellout backend, both found adversarially reviewing the switch to ComposeBackendNative as the only backend in dap: 1. Non-primary services with `build:` were never built — the orchestrator only creates containers from images and the pull-retry is gated on a non-empty reference, so a `build:` sidecar reached ContainerCreate with an empty image. Engine.Up now builds every selected sidecar before the orchestrator runs (image:+build: tags the built image with image:, build-only gets compose v2's - default) and pins the result via ApplyBuildOverride, so hashing, drift detection and pull-retry all see a concrete reference. 2. ensureService recreated any existing container whose dev.containers.config-hash / image-digest labels did not match — including containers the orchestrator did not create (shellout backend, plain docker compose), which have no such labels at all. Removing them destroys the writable layer the shellout path's NoRecreate contract preserved across restarts. Such containers are now adopted: started if stopped, otherwise left alone. Drift-based recreation still applies to containers carrying our labels, and Recreate-mode Ups still tear the whole project down first. Co-Authored-By: Claude Fable 5 --- CHANGELOG.md | 19 +++++ compose/apply_override.go | 21 +++--- compose/orchestrator.go | 44 ++++++++---- compose/orchestrator_test.go | 69 ++++++++++++++++++ up.go | 76 +++++++++++++++++++- up_compose_sidecar_test.go | 131 +++++++++++++++++++++++++++++++++++ 6 files changed, 335 insertions(+), 25 deletions(-) create mode 100644 up_compose_sidecar_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 354ba44..985e0c4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,25 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed + +- **compose (native)** — non-primary services that declare `build:` are + now built before the orchestrator runs, matching the shellout backend + (where `docker compose up` builds them implicitly). Previously a + `build:` sidecar reached `ContainerCreate` with an empty image and the + Up failed. Compose semantics are preserved: `image:` + `build:` tags + the built image with `image:`, a build-only service gets compose v2's + default `-` name. +- **compose (native)** — containers the native orchestrator did not + create (shellout backend, plain `docker compose up`) are now adopted + on a non-recreate Up instead of being stopped and removed. Without the + `dev.containers.config-hash` / `image-digest` labels there is no drift + to detect, and removal destroyed the writable layer (in-container + `$HOME` and friends) that the shellout path's `NoRecreate` contract + preserved across restarts — a data-loss hazard for every workspace + migrating from the shellout backend. Recreate-mode Ups still tear the + whole project down first, so forced refreshes behave as before. + ## [0.4.0] - 2026-06-24 ### Added diff --git a/compose/apply_override.go b/compose/apply_override.go index f201d1e..2bb1665 100644 --- a/compose/apply_override.go +++ b/compose/apply_override.go @@ -15,25 +15,26 @@ import ( // existing WriteBuildOverride / WriteRunOverride file emitters // until PR17 deletes that path. -// ApplyBuildOverride mutates project so the primary service's image -// is pinned to imageRef and any build: directive is cleared. Mirrors +// ApplyBuildOverride mutates project so the named service's image is +// pinned to imageRef and any build: directive is cleared. Mirrors // WriteBuildOverride's behavior; safe to call on a freshly loaded -// project. +// project. Engine.Up applies it to the primary service (with the +// feature-extended image) and to every sidecar it built. // -// Returns an error if the primary service is missing. -func ApplyBuildOverride(project *composetypes.Project, primaryService, imageRef string) error { +// Returns an error if the service is missing. +func ApplyBuildOverride(project *composetypes.Project, service, imageRef string) error { if project == nil { return fmt.Errorf("ApplyBuildOverride: nil project") } - if primaryService == "" { - return fmt.Errorf("ApplyBuildOverride: primaryService required") + if service == "" { + return fmt.Errorf("ApplyBuildOverride: service required") } if imageRef == "" { return fmt.Errorf("ApplyBuildOverride: imageRef required") } - svc, ok := project.Services[primaryService] + svc, ok := project.Services[service] if !ok { - return fmt.Errorf("ApplyBuildOverride: primary service %q not found in project", primaryService) + return fmt.Errorf("ApplyBuildOverride: service %q not found in project", service) } svc.Image = imageRef // Compose v2 keeps Image and Build mutually exclusive at orchestration @@ -41,7 +42,7 @@ func ApplyBuildOverride(project *composetypes.Project, primaryService, imageRef // in the YAML override. compose-go represents the field as a pointer // so nil = unset. svc.Build = nil - project.Services[primaryService] = svc + project.Services[service] = svc return nil } diff --git a/compose/orchestrator.go b/compose/orchestrator.go index b17c8d7..bda0226 100644 --- a/compose/orchestrator.go +++ b/compose/orchestrator.go @@ -479,21 +479,39 @@ func (o *Orchestrator) ensureService( // might match by accident (e.g. a digest probe that returned // empty). details, ierr := o.rt.InspectContainer(ctx, c.ID) - if ierr == nil && details != nil && - details.Labels[LabelConfigHash] == hash && - details.Labels[LabelImageDigest] == imageDigest { - // Config and image match — the container is reusable. - // If it's not currently Running (e.g. dockerd was just - // restarted and brought stopped containers back from - // on-disk state), start it instead of destroying it. - // Recreating would lose the writable layer (the user's - // $HOME inside the container, etc.) — see issue #71. - if c.State != runtime.StateRunning { - if err := o.rt.StartContainer(ctx, c.ID); err != nil { - return "", fmt.Errorf("StartContainer(%s): %w", svc.Name, err) + if ierr == nil && details != nil { + if _, managed := details.Labels[LabelConfigHash]; !managed { + // A container for this (project, service) that this + // orchestrator did not create — the shellout backend or a + // plain `docker compose up`. Adopt it instead of + // recreating: without our labels there is no stored hash + // to detect drift against, and removing it would destroy + // the writable layer (the user's $HOME inside the + // container, etc.) that the shellout path's NoRecreate + // contract preserved across restarts. A caller that wants + // these replaced asks for it explicitly: a Recreate-mode + // Up tears the whole project down before reaching here. + if c.State != runtime.StateRunning { + if err := o.rt.StartContainer(ctx, c.ID); err != nil { + return "", fmt.Errorf("StartContainer(adopted %s): %w", svc.Name, err) + } + } + return c.ID, nil + } + if details.Labels[LabelConfigHash] == hash && + details.Labels[LabelImageDigest] == imageDigest { + // Config and image match — the container is reusable. + // If it's not currently Running (e.g. dockerd was just + // restarted and brought stopped containers back from + // on-disk state), start it instead of destroying it. + // Recreating would lose the writable layer — see issue #71. + if c.State != runtime.StateRunning { + if err := o.rt.StartContainer(ctx, c.ID); err != nil { + return "", fmt.Errorf("StartContainer(%s): %w", svc.Name, err) + } } + return c.ID, nil } - return c.ID, nil } // Different config — recreate. _ = o.rt.StopContainer(ctx, c.ID, runtime.StopOptions{Timeout: 10 * time.Second}) diff --git a/compose/orchestrator_test.go b/compose/orchestrator_test.go index 01f9b86..a8f9c23 100644 --- a/compose/orchestrator_test.go +++ b/compose/orchestrator_test.go @@ -898,3 +898,72 @@ func TestDown_Idempotent(t *testing.T) { t.Errorf("Down on empty: %v", err) } } + +// A container for the (project, service) pair that this orchestrator +// did not create — the shellout backend or a plain `docker compose up` +// — has the compose labels but none of the dev.containers ones. It +// must be adopted, not recreated: removing it destroys the writable +// layer the shellout path's NoRecreate contract preserved. Recreate- +// mode Ups tear the project down before the orchestrator runs, so +// adoption only ever applies to reattach/resume flows. +func TestUp_AdoptsForeignContainerWithoutOurLabels(t *testing.T) { + rt := newMockRuntime() + orch := NewOrchestrator(rt, "docker") + proj := newProject(t, map[string][]string{"app": nil}) + + rt.containers["legacy-1"] = &mockContainer{ + id: "legacy-1", name: "dc-x-app-1", image: "alpine", + labels: map[string]string{ + LabelComposeProject: "dc-x", + LabelComposeService: "app", + }, + state: runtime.StateExited, + } + + res, err := orch.Up(context.Background(), &Plan{Project: proj, ProjectName: "dc-x"}) + if err != nil { + t.Fatalf("Up: %v", err) + } + if res.ContainerIDs["app"] != "legacy-1" { + t.Errorf("ContainerIDs[app] = %q, want the adopted legacy-1", res.ContainerIDs["app"]) + } + if rt.removeCalls != 0 { + t.Errorf("removeCalls = %d; adopting must not remove the foreign container", rt.removeCalls) + } + if rt.runCalls != 0 { + t.Errorf("runCalls = %d; adopting must not create a replacement", rt.runCalls) + } + // The exited container must be started, mirroring the reuse path. + if rt.containers["legacy-1"].state != runtime.StateRunning { + t.Errorf("adopted container state = %v, want running", rt.containers["legacy-1"].state) + } +} + +// Same adoption when the foreign container is already running: fully +// hands-off. +func TestUp_AdoptsRunningForeignContainerWithoutStarting(t *testing.T) { + rt := newMockRuntime() + orch := NewOrchestrator(rt, "docker") + proj := newProject(t, map[string][]string{"app": nil}) + + rt.containers["legacy-2"] = &mockContainer{ + id: "legacy-2", name: "dc-x-app-1", image: "alpine", + labels: map[string]string{ + LabelComposeProject: "dc-x", + LabelComposeService: "app", + }, + state: runtime.StateRunning, + } + + res, err := orch.Up(context.Background(), &Plan{Project: proj, ProjectName: "dc-x"}) + if err != nil { + t.Fatalf("Up: %v", err) + } + if res.ContainerIDs["app"] != "legacy-2" { + t.Errorf("ContainerIDs[app] = %q, want legacy-2", res.ContainerIDs["app"]) + } + if rt.removeCalls != 0 || rt.runCalls != 0 || rt.startCalls != 0 { + t.Errorf("adoption of a running container must be hands-off (remove=%d run=%d start=%d)", + rt.removeCalls, rt.runCalls, rt.startCalls) + } +} diff --git a/up.go b/up.go index cc4f40e..1675030 100644 --- a/up.go +++ b/up.go @@ -5,6 +5,8 @@ import ( "fmt" "os" "path/filepath" + "slices" + "sort" "time" composetypes "github.com/compose-spec/compose-go/v2/types" @@ -605,7 +607,7 @@ func (e *Engine) createFreshCompose(ctx context.Context, cfg *config.ResolvedCon switch e.opts.ComposeBackend { case ComposeBackendNative: return e.upComposeNative(ctx, cfg, opts, project, src, projectName, - finalImage, runOverride) + workingDir, finalImage, runOverride) case ComposeBackendShellout: return e.upComposeShellout(ctx, cfg, opts, project, src, projectName, workingDir, finalImage, runOverride, existingContainer) @@ -698,12 +700,15 @@ func (e *Engine) upComposeNative( opts UpOptions, project *composetypes.Project, src *config.ComposeSource, - projectName, finalImage string, + projectName, workingDir, finalImage string, runOverride compose.Override, ) (*Workspace, error) { if err := compose.ApplyBuildOverride(project, src.Service, finalImage); err != nil { return nil, err } + if err := e.buildComposeSidecarImages(ctx, project, src, projectName, workingDir, opts); err != nil { + return nil, err + } if err := compose.ApplyRunOverride(project, src.Service, runOverride); err != nil { return nil, err } @@ -757,6 +762,73 @@ func composeBindMounts(cfg *config.ResolvedConfig, opts UpOptions) []compose.Bin return out } +// buildComposeSidecarImages builds every selected non-primary service +// that declares `build:`. The shellout backend delegated these to +// `docker compose up`, which builds missing images implicitly; the +// native orchestrator only creates containers from images, so the +// builds must happen before it runs. Compose semantics are preserved: +// a service with both `image:` and `build:` gets the built image +// tagged with its `image:`, a build-only service gets compose v2's +// default `-` name. Either way the service's +// `build:` is cleared and `image:` set, so the orchestrator's hash, +// pull-retry, and drift checks all see a concrete reference. +func (e *Engine) buildComposeSidecarImages( + ctx context.Context, + project *composetypes.Project, + src *config.ComposeSource, + projectName, workingDir string, + opts UpOptions, +) error { + selected := func(name string) bool { + if len(src.RunServices) == 0 { + return true + } + return slices.Contains(src.RunServices, name) + } + // Deterministic build order: map iteration would shuffle build + // output (and any failure) between runs. + names := make([]string, 0, len(project.Services)) + for name := range project.Services { + names = append(names, name) + } + sort.Strings(names) + + for _, name := range names { + svc := project.Services[name] + // The primary service's build already happened in + // prepareComposeServiceImage (with feature layering on top); + // ApplyBuildOverride cleared its Build field before this runs. + if name == src.Service || svc.Build == nil || !selected(name) { + continue + } + tag := svc.Image + if tag == "" { + tag = projectName + "-" + name + } + ctxPath := svc.Build.Context + if ctxPath == "" { + ctxPath = workingDir + } + if !filepath.IsAbs(ctxPath) { + ctxPath = filepath.Join(workingDir, ctxPath) + } + opts.bus.Emit(events.BuildStartEvent{Source: events.BuildSourceDockerfile, Ref: tag}) + if _, err := e.runtime.BuildImage(ctx, runtime.BuildSpec{ + ContextPath: ctxPath, + Dockerfile: svc.Build.Dockerfile, + Tag: tag, + Args: flattenStringMap(svc.Build.Args), + Target: svc.Build.Target, + }, opts.bus.BuildChan(events.BuildSourceDockerfile)); err != nil { + return fmt.Errorf("build compose service %q image: %w", name, err) + } + if err := compose.ApplyBuildOverride(project, name, tag); err != nil { + return err + } + } + return nil +} + // prepareComposeServiceImage resolves the base image for a compose // primary service: either the service's `image:` directive (pulled if // missing locally) or the result of building its `build:` directive. diff --git a/up_compose_sidecar_test.go b/up_compose_sidecar_test.go new file mode 100644 index 0000000..ac993bd --- /dev/null +++ b/up_compose_sidecar_test.go @@ -0,0 +1,131 @@ +package devcontainer + +import ( + "context" + "path/filepath" + "testing" + + composetypes "github.com/compose-spec/compose-go/v2/types" + + "github.com/crunchloop/devcontainer/config" + "github.com/crunchloop/devcontainer/events" + "github.com/crunchloop/devcontainer/runtime" +) + +// buildRecorder wraps fakeRuntime with a BuildImage that succeeds and +// records every spec, so sidecar-build tests can assert what was built +// without a real backend. +type buildRecorder struct { + *fakeRuntime + builds []runtime.BuildSpec +} + +func (b *buildRecorder) BuildImage(ctx context.Context, spec runtime.BuildSpec, ch chan<- runtime.BuildEvent) (runtime.ImageRef, error) { + b.builds = append(b.builds, spec) + return runtime.ImageRef{ID: "sha256:" + spec.Tag, Tags: []string{spec.Tag}}, nil +} + +func sidecarProject(services map[string]composetypes.ServiceConfig) *composetypes.Project { + svcs := composetypes.Services{} + for name, svc := range services { + svc.Name = name + svcs[name] = svc + } + return &composetypes.Project{Services: svcs} +} + +func sidecarEngine(t *testing.T) (*Engine, *buildRecorder) { + t.Helper() + rt := &buildRecorder{fakeRuntime: newFakeRuntime()} + eng, err := New(EngineOptions{Runtime: rt, ComposeBackend: ComposeBackendNative}) + if err != nil { + t.Fatalf("New: %v", err) + } + return eng, rt +} + +func sidecarUpOptions() UpOptions { + opts := UpOptions{} + opts.bus = newEventBus(events.NewEmitter(nil), nil) + return opts +} + +// The shellout backend delegated sidecar `build:` services to +// `docker compose up`; the native orchestrator only creates containers +// from images, so the engine must build them first — a build-only +// sidecar previously reached ContainerCreate with an empty image. +func TestBuildComposeSidecarImages_BuildsNonPrimaryServices(t *testing.T) { + eng, rt := sidecarEngine(t) + workingDir := t.TempDir() + + project := sidecarProject(map[string]composetypes.ServiceConfig{ + // Primary: ApplyBuildOverride has already cleared Build by the + // time the helper runs; simulate that state. + "app": {Image: "dc-final:latest"}, + // Build-only sidecar → compose v2's default -. + "db": {Build: &composetypes.BuildConfig{Context: "./db", Dockerfile: "Dockerfile"}}, + // image: + build: → the built image is tagged with image:. + "worker": {Image: "acme/worker:dev", Build: &composetypes.BuildConfig{Context: "./worker"}}, + // Plain image sidecar → untouched. + "cache": {Image: "redis:7"}, + }) + src := &config.ComposeSource{Service: "app"} + + if err := eng.buildComposeSidecarImages(context.Background(), project, src, "dc-x", workingDir, sidecarUpOptions()); err != nil { + t.Fatalf("buildComposeSidecarImages: %v", err) + } + + if len(rt.builds) != 2 { + t.Fatalf("builds = %d, want 2 (db, worker); specs: %+v", len(rt.builds), rt.builds) + } + // Deterministic order: sorted by service name. + if rt.builds[0].Tag != "dc-x-db" { + t.Errorf("db build tag = %q, want dc-x-db", rt.builds[0].Tag) + } + if got, want := rt.builds[0].ContextPath, filepath.Join(workingDir, "db"); got != want { + t.Errorf("db build context = %q, want %q", got, want) + } + if rt.builds[1].Tag != "acme/worker:dev" { + t.Errorf("worker build tag = %q, want acme/worker:dev", rt.builds[1].Tag) + } + + // The project must end up image-only so the orchestrator's hash, + // pull-retry, and drift checks all see a concrete reference. + for name, wantImage := range map[string]string{ + "db": "dc-x-db", + "worker": "acme/worker:dev", + "cache": "redis:7", + } { + svc := project.Services[name] + if svc.Image != wantImage { + t.Errorf("service %q image = %q, want %q", name, svc.Image, wantImage) + } + if svc.Build != nil { + t.Errorf("service %q still carries build:", name) + } + } +} + +// runServices restricts which services come up; services outside the +// selection must not be built either. +func TestBuildComposeSidecarImages_HonorsRunServices(t *testing.T) { + eng, rt := sidecarEngine(t) + + project := sidecarProject(map[string]composetypes.ServiceConfig{ + "app": {Image: "dc-final:latest"}, + "db": {Build: &composetypes.BuildConfig{Context: "./db"}}, + "excluded": {Build: &composetypes.BuildConfig{Context: "./excluded"}}, + }) + src := &config.ComposeSource{Service: "app", RunServices: []string{"db"}} + + if err := eng.buildComposeSidecarImages(context.Background(), project, src, "dc-x", t.TempDir(), sidecarUpOptions()); err != nil { + t.Fatalf("buildComposeSidecarImages: %v", err) + } + + if len(rt.builds) != 1 || rt.builds[0].Tag != "dc-x-db" { + t.Fatalf("builds = %+v, want exactly the selected db service", rt.builds) + } + if svc := project.Services["excluded"]; svc.Build == nil { + t.Error("unselected service was mutated") + } +}