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") + } +}