Skip to content

Commit 3c09737

Browse files
fix: tear down the container before wiping the cache it mounts (#20)
Three defects in the Destroy rework, found reviewing it after the fact. Destroy removed the by-name container *after* emptying cacheLocation. In container mode Init bind-mounts the venv and the uv cache out of that very directory, so on the one path that reaches a container by name — a runtime holding no environment of its own, which is what shutdown reaches when Init never ran in this process — the wipe deleted the interpreter out from under a live container and only then removed it. That is the bug this branch set out to fix, left standing on the container path while the commit message claimed the cache is cleared only once execution has ended. All execution teardown now precedes destructive cleanup, and the container is skipped when shutting down the owned environment already removed it. Destroy reconstructed that environment with the default image, ignoring the runtime-image override Init honors. NewDockerEnvironment pulls the image it is handed, so with an override set Destroy pulled the wrong one, and an image it could not obtain became an error that left the container running. Both call sites now resolve the image the same way, and Destroy falls back to the default rather than let an unparseable override strand a container: the lookup is by container name, never by image. Destroy could not stop a Start already on its way — the watcher's debounce can still fire one after StopWatcher. Releasing the environment made that Start fail on its own, but a Destroy whose environment shutdown failed keeps the environment reachable for a retry, and a Start landing there launched a replacement against the cache Destroy had just wiped. Destroy now marks the runtime torn down and Start consults it; Init reopens the lifecycle so a deliberate re-init still works. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 9480240 commit 3c09737

2 files changed

Lines changed: 222 additions & 32 deletions

File tree

‎destroy_test.go‎

Lines changed: 137 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ package main
33
import (
44
"context"
55
"errors"
6+
"fmt"
67
"os"
78
"path/filepath"
89
"testing"
@@ -12,6 +13,8 @@ import (
1213
runtimev0 "github.com/codefly-dev/core/generated/go/codefly/services/runtime/v0"
1314
"github.com/codefly-dev/core/resources"
1415
runners "github.com/codefly-dev/core/runners/base"
16+
dockerrun "github.com/codefly-dev/core/runners/dockerrun"
17+
"github.com/codefly-dev/core/shared"
1518
"github.com/stretchr/testify/require"
1619
)
1720

@@ -208,3 +211,137 @@ func TestDestroyStopsRealUvicornWithoutStop(t *testing.T) {
208211
require.False(t, running, "uvicorn must be gone before Destroy reports success")
209212
require.False(t, serves(runtime.port), "the listener must be gone before Destroy reports success")
210213
}
214+
215+
// unstoppableEnv refuses to shut down, the state Destroy deliberately leaves
216+
// reachable for a retry — and therefore the state in which a late Start still
217+
// has somewhere to launch a process.
218+
type unstoppableEnv struct {
219+
runners.RunnerEnvironment
220+
processes int
221+
}
222+
223+
func (unstoppableEnv) Shutdown(context.Context) error {
224+
return errors.New("environment will not shut down")
225+
}
226+
227+
func (e *unstoppableEnv) NewProcess(string, ...string) (runners.Proc, error) {
228+
e.processes++
229+
return &fakeProc{}, nil
230+
}
231+
232+
func TestStartAfterFailedDestroyCannotLaunchReplacement(t *testing.T) {
233+
ctx := context.Background()
234+
runtime, _, _ := destroyRuntime(t)
235+
env := &unstoppableEnv{}
236+
runtime.runnerEnvironment = env
237+
238+
resp, err := runtime.Destroy(ctx, &runtimev0.DestroyRequest{})
239+
require.NoError(t, err)
240+
require.Equal(t, runtimev0.DestroyStatus_ERROR, resp.GetStatus().GetState())
241+
require.NotNil(t, runtime.runnerEnvironment,
242+
"a failed shutdown must keep the environment reachable for a retry")
243+
244+
start, err := runtime.Start(ctx, &runtimev0.StartRequest{})
245+
require.NoError(t, err)
246+
require.Equal(t, runtimev0.StartStatus_ERROR, start.GetStatus().GetState())
247+
require.Zero(t, env.processes, "no replacement process may be launched after Destroy")
248+
require.Nil(t, currentRunner(runtime))
249+
}
250+
251+
// TestInitReopensLifecycleAfterDestroy pins the other half of that contract: a
252+
// deliberate Init is a new lifecycle, so Start works again rather than being
253+
// refused forever by a runtime nobody can revive.
254+
func TestInitReopensLifecycleAfterDestroy(t *testing.T) {
255+
ctx := context.Background()
256+
runtime, _, _ := destroyRuntime(t)
257+
258+
_, err := runtime.Destroy(ctx, &runtimev0.DestroyRequest{})
259+
require.NoError(t, err)
260+
261+
runtime.runnerMu.Lock()
262+
require.True(t, runtime.destroyed)
263+
runtime.runnerMu.Unlock()
264+
265+
// Init fails well before it would need a real environment; what matters is
266+
// that it reopened the lifecycle on its way through.
267+
_, _ = runtime.Init(ctx, &runtimev0.InitRequest{RuntimeContext: resources.NewRuntimeContextNative()})
268+
269+
runtime.runnerMu.Lock()
270+
defer runtime.runnerMu.Unlock()
271+
require.False(t, runtime.destroyed, "Init must reopen the lifecycle it is starting")
272+
}
273+
274+
func TestResolveRuntimeImageHonorsOverride(t *testing.T) {
275+
runtime := NewRuntime(NewService())
276+
277+
image, err := runtime.resolveRuntimeImage()
278+
require.NoError(t, err)
279+
require.Equal(t, runtimeImage, image)
280+
281+
runtime.FastAPI.Settings.RuntimeImage = "ghcr.io/acme/python:1.2.3"
282+
image, err = runtime.resolveRuntimeImage()
283+
require.NoError(t, err)
284+
require.Equal(t, "ghcr.io/acme/python", image.Name)
285+
require.Equal(t, "1.2.3", image.Tag,
286+
"Destroy must reach for the image the service actually runs, not the default")
287+
288+
runtime.FastAPI.Settings.RuntimeImage = "ghcr.io/acme/python:latest"
289+
_, err = runtime.resolveRuntimeImage()
290+
require.Error(t, err, "an unpinned override stays rejected")
291+
}
292+
293+
// TestDestroyRemovesContainerBeforeClearingCache is the container half of the
294+
// ordering contract. cacheLocation is the directory Init bind-mounts the venv
295+
// and the uv cache out of, so wiping it while the container is up deletes the
296+
// interpreter under the running service. runnerEnvironment is deliberately nil:
297+
// that is the shape `codefly` shutdown reaches when Init never ran in this
298+
// process, and the only path on which Destroy reaches the container by name.
299+
func TestDestroyRemovesContainerBeforeClearingCache(t *testing.T) {
300+
ctx := context.Background()
301+
302+
// Docker on macOS only bind-mounts shared paths, and the per-user temp dir
303+
// is not one — keep the mount inside the repo, as the lifecycle tests do.
304+
cache, err := os.MkdirTemp("testdata", "destroy-cache-")
305+
require.NoError(t, err)
306+
t.Cleanup(func() { _ = os.RemoveAll(cache) })
307+
cache = shared.MustSolvePath(cache)
308+
309+
runtime := NewRuntime(NewService())
310+
runtime.Location = t.TempDir()
311+
runtime.Logger = runtime.Wool
312+
runtime.Base.Runtime.RuntimeContext = resources.NewRuntimeContextContainer()
313+
runtime.Identity = &resources.ServiceIdentity{
314+
Name: fmt.Sprintf("destroy-order-%d", time.Now().UnixMilli()),
315+
Module: "mod",
316+
Workspace: "test",
317+
WorkspacePath: runtime.Location,
318+
}
319+
runtime.Environment = shared.Must(resources.LocalEnvironment().Proto())
320+
runtime.cacheLocation = cache
321+
322+
env, err := dockerrun.NewDockerEnvironment(ctx, runtimeImage, t.TempDir(), runtime.Base.Runtime.UniqueWithWorkspace())
323+
if err != nil {
324+
t.Skipf("docker is not available: %v", err)
325+
}
326+
env.WithPause()
327+
env.WithMount(cache, "/cache")
328+
require.NoError(t, env.Init(ctx))
329+
t.Cleanup(func() { _ = env.Shutdown(context.Background()) })
330+
331+
proc, err := env.NewProcess("sh", "-c", "while true; do touch /cache/heartbeat; sleep 0.05; done")
332+
require.NoError(t, err)
333+
require.NoError(t, proc.Start(ctx))
334+
require.Eventually(t, func() bool {
335+
entries, readErr := os.ReadDir(cache)
336+
return readErr == nil && len(entries) > 0
337+
}, 30*time.Second, 100*time.Millisecond, "the container must be writing into the mounted cache")
338+
339+
require.Nil(t, runtime.runnerEnvironment)
340+
resp, err := runtime.Destroy(ctx, &runtimev0.DestroyRequest{})
341+
require.NoError(t, err)
342+
require.Equal(t, runtimev0.DestroyStatus_SUCCESS, resp.GetStatus().GetState(), resp.GetStatus().GetMessage())
343+
344+
entries, err := os.ReadDir(cache)
345+
require.NoError(t, err)
346+
require.Empty(t, entries, "the container must be gone before the cache it mounts is wiped")
347+
}

‎runtime.go‎

Lines changed: 85 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,12 @@ type Runtime struct {
8383
grpcPort uint16
8484

8585
cacheLocation string
86+
87+
// destroyed marks the runtime as torn down, guarded by runnerMu. Destroy
88+
// cannot stop a Start that is already on its way — the watcher's debounce
89+
// can still fire one after StopWatcher — so Start consults this rather
90+
// than launching a process against resources Destroy is removing.
91+
destroyed bool
8692
}
8793

8894
func NewRuntime(svc *Service) *Runtime {
@@ -138,19 +144,27 @@ func (s *Runtime) DockerEnvPath() string {
138144
return path.Join(s.Location, ".cache/container/.venv")
139145
}
140146

147+
// resolveRuntimeImage returns the image the service runs in: the settings
148+
// override when one is set, else the codefly-built default. Strict pinning —
149+
// :latest and untagged refs are rejected so builds stay reproducible.
150+
func (s *Runtime) resolveRuntimeImage() (*resources.DockerImage, error) {
151+
override := s.FastAPI.Settings.RuntimeImage
152+
if override == "" {
153+
return runtimeImage, nil
154+
}
155+
parsed, err := resources.ParsePinnedImage(override)
156+
if err != nil {
157+
return nil, s.Wool.Wrapf(err, "invalid docker-image override in service.codefly.yaml")
158+
}
159+
s.Wool.Info("using docker-image override (not recommended)", wool.Field("image", parsed.FullName()))
160+
return parsed, nil
161+
}
162+
141163
func (s *Runtime) CreateRunnerEnvironment(ctx context.Context) error {
142164
s.Wool.Debug("creating runner environment in", wool.DirField(s.Identity.WorkspacePath))
143-
// Resolve the runtime image: settings override (if any) takes priority,
144-
// else fall back to the codefly-built default. Strict pinning — we
145-
// reject :latest and untagged refs so builds stay reproducible.
146-
image := runtimeImage
147-
if override := s.FastAPI.Settings.RuntimeImage; override != "" {
148-
parsed, perr := resources.ParsePinnedImage(override)
149-
if perr != nil {
150-
return s.Wool.Wrapf(perr, "invalid docker-image override in service.codefly.yaml")
151-
}
152-
s.Wool.Info("using docker-image override (not recommended)", wool.Field("image", parsed.FullName()))
153-
image = parsed
165+
image, err := s.resolveRuntimeImage()
166+
if err != nil {
167+
return err
154168
}
155169

156170
switch {
@@ -260,6 +274,12 @@ func (s *Runtime) Init(ctx context.Context, req *runtimev0.InitRequest) (*runtim
260274

261275
s.Base.Runtime.LogInitRequest(req)
262276

277+
// Init opens a new lifecycle, so a runtime torn down earlier accepts work
278+
// again — otherwise Init would succeed and every later Start refuse.
279+
s.runnerMu.Lock()
280+
s.destroyed = false
281+
s.runnerMu.Unlock()
282+
263283
if err := s.SetRuntimeContext(ctx, req.RuntimeContext); err != nil {
264284
return s.Base.Runtime.InitError(err)
265285
}
@@ -378,6 +398,10 @@ func (s *Runtime) Start(ctx context.Context, req *runtimev0.StartRequest) (*runt
378398

379399
s.Base.Runtime.LogStartRequest(req)
380400

401+
if s.destroyed {
402+
return s.Base.Runtime.StartError(s.Wool.NewError("service has been destroyed (Init must run before it can start again)"))
403+
}
404+
381405
inputs := startInputs(req)
382406

383407
if s.runner != nil {
@@ -769,7 +793,10 @@ func withOverrides(envs []*resources.EnvironmentVariable, overrides map[string]s
769793
// earlier one fails, so one stubborn resource cannot strand the rest, and the
770794
// environment is forgotten only once it is actually down — a caller retrying
771795
// after a failure must still be able to reach it.
772-
func (s *Runtime) endExecution(ctx context.Context) error {
796+
//
797+
// It reports whether the owned environment was actually shut down, which in
798+
// container mode is what tells Destroy the container is already gone.
799+
func (s *Runtime) endExecution(ctx context.Context) (bool, error) {
773800
// Cancel the watcher and let its Start goroutine's deferred close of Events
774801
// run exactly once — never close Events here, or it races that goroutine
775802
// into a "close of closed channel" panic.
@@ -779,18 +806,20 @@ func (s *Runtime) endExecution(ctx context.Context) error {
779806
if err := s.stopRunner(ctx); err != nil {
780807
errs = append(errs, s.Wool.Wrapf(err, "cannot stop the fastapi app"))
781808
}
809+
released := false
782810
if s.runnerEnvironment != nil {
783811
if err := s.runnerEnvironment.Shutdown(ctx); err != nil {
784812
errs = append(errs, s.Wool.Wrapf(err, "cannot shut down the runner environment"))
785813
} else {
786814
// A shut-down environment cannot serve another process — a docker
787815
// one has closed its client — so drop it and let a later Init build
788816
// a fresh one rather than hand Code and the REPL a dead handle.
817+
released = true
789818
s.runnerEnvironment = nil
790819
s.FastAPI.Service.ActiveEnv = nil
791820
}
792821
}
793-
return errors.Join(errs...)
822+
return released, errors.Join(errs...)
794823
}
795824

796825
// Stop releases the execution resources and keeps everything on disk, so a
@@ -803,7 +832,7 @@ func (s *Runtime) Stop(ctx context.Context, req *runtimev0.StopRequest) (*runtim
803832
defer s.runnerMu.Unlock()
804833

805834
s.Wool.Debug("stopping service")
806-
if err := s.endExecution(ctx); err != nil {
835+
if _, err := s.endExecution(ctx); err != nil {
807836
return s.Base.Runtime.StopError(err)
808837
}
809838
return s.Base.Runtime.StopResponse()
@@ -823,31 +852,34 @@ func (s *Runtime) Destroy(ctx context.Context, req *runtimev0.DestroyRequest) (*
823852
var errs []error
824853

825854
s.runnerMu.Lock()
826-
if err := s.endExecution(ctx); err != nil {
855+
s.destroyed = true
856+
released, err := s.endExecution(ctx)
857+
if err != nil {
827858
errs = append(errs, err)
828859
}
860+
cacheLocation := s.cacheLocation
829861
s.runnerMu.Unlock()
830862

831-
// Only once execution has ended: a process still running against the cache
832-
// repopulates what this removes, and is reading the venv being pulled out
833-
// from under it. An empty location means Init never claimed one — there is
834-
// nothing owned to clear, and EmptyDir rejects the empty path as an error.
835-
if s.cacheLocation != "" {
836-
s.Wool.Debug("removing cache")
837-
if err := shared.EmptyDir(ctx, s.cacheLocation); err != nil {
838-
errs = append(errs, s.Wool.Wrapf(err, "cannot remove cache"))
863+
// A container outlives the agent that started it, so a Destroy whose
864+
// runtime holds no environment — a fresh agent, or an Init that never ran —
865+
// still has to reach it by name. Shutting the owned environment down has
866+
// already removed it, and reaching for docker again there would only invent
867+
// a way for a finished teardown to fail.
868+
if s.Base.Runtime.IsContainerRuntime() && !released {
869+
if err := s.destroyContainerByIdentity(ctx); err != nil {
870+
errs = append(errs, err)
839871
}
840872
}
841873

842-
if s.Base.Runtime.IsContainerRuntime() {
843-
// A container outlives the agent that started it, so a Destroy whose
844-
// runtime never held one still has to reach it by name.
845-
s.Wool.Debug("running in container")
846-
dockerEnv, err := dockerrun.NewDockerEnvironment(ctx, runtimeImage, s.Service.SourceLocation, s.Base.Runtime.UniqueWithWorkspace())
847-
if err != nil {
848-
errs = append(errs, s.Wool.Wrapf(err, "cannot reach the container environment"))
849-
} else if err := dockerEnv.Shutdown(ctx); err != nil {
850-
errs = append(errs, s.Wool.Wrapf(err, "cannot shut down the container environment"))
874+
// Last, and only once nothing is executing any more. In container mode the
875+
// venv and the uv cache are bind-mounted out of this directory, so wiping
876+
// it while the container is still up pulls the interpreter out from under
877+
// the running service. An empty location means Init never claimed one —
878+
// nothing owned to clear, and EmptyDir rejects the empty path as an error.
879+
if cacheLocation != "" {
880+
s.Wool.Debug("removing cache")
881+
if err := shared.EmptyDir(ctx, cacheLocation); err != nil {
882+
errs = append(errs, s.Wool.Wrapf(err, "cannot remove cache"))
851883
}
852884
}
853885

@@ -857,6 +889,27 @@ func (s *Runtime) Destroy(ctx context.Context, req *runtimev0.DestroyRequest) (*
857889
return s.Base.Runtime.DestroyResponse()
858890
}
859891

892+
// destroyContainerByIdentity removes the service's container by the name it is
893+
// registered under, for a Destroy holding no environment of its own.
894+
func (s *Runtime) destroyContainerByIdentity(ctx context.Context) error {
895+
s.Wool.Debug("running in container")
896+
image, err := s.resolveRuntimeImage()
897+
if err != nil {
898+
// The container is found by name, never by image, so an override this
899+
// agent cannot parse must not be what leaves it running.
900+
s.Wool.Warn("cannot resolve the runtime image, falling back to the default", wool.ErrField(err))
901+
image = runtimeImage
902+
}
903+
dockerEnv, err := dockerrun.NewDockerEnvironment(ctx, image, s.Service.SourceLocation, s.Base.Runtime.UniqueWithWorkspace())
904+
if err != nil {
905+
return s.Wool.Wrapf(err, "cannot reach the container environment")
906+
}
907+
if err := dockerEnv.Shutdown(ctx); err != nil {
908+
return s.Wool.Wrapf(err, "cannot shut down the container environment")
909+
}
910+
return nil
911+
}
912+
860913
func (s *Runtime) EventHandler(event code.Change) error {
861914
if strings.Contains(event.Path, "api.json") {
862915
return nil

0 commit comments

Comments
 (0)