Skip to content

Commit eb917be

Browse files
fix: make Destroy self-contained for execution cleanup (#20) (#22)
* fix: make Destroy self-contained for execution cleanup (#20) Destroy stopped the runner but left the file watcher running and never shut down the runner environment, and it reported SUCCESS after merely logging a failed process stop or cache wipe. Factor the teardown Stop and Destroy share into endExecution: stop the watcher first, then the process generation, then the owned environment, attempting every step even when an earlier one fails and aggregating the errors so the caller sees them. The environment is released only once it is actually down, so a retry can still reach it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * 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> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 43e5eae commit eb917be

2 files changed

Lines changed: 480 additions & 39 deletions

File tree

‎destroy_test.go‎

Lines changed: 347 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,347 @@
1+
package main
2+
3+
import (
4+
"context"
5+
"errors"
6+
"fmt"
7+
"os"
8+
"path/filepath"
9+
"testing"
10+
"time"
11+
12+
"github.com/codefly-dev/core/agents/services"
13+
runtimev0 "github.com/codefly-dev/core/generated/go/codefly/services/runtime/v0"
14+
"github.com/codefly-dev/core/resources"
15+
runners "github.com/codefly-dev/core/runners/base"
16+
dockerrun "github.com/codefly-dev/core/runners/dockerrun"
17+
"github.com/codefly-dev/core/shared"
18+
"github.com/stretchr/testify/require"
19+
)
20+
21+
// destroyRuntime is a Runtime holding a real, running native process — the
22+
// shape `codefly` shutdown reaches when it calls Destroy with no preceding
23+
// Stop. The process touches a file in cacheLocation every 50ms, so a cache
24+
// wiped while it was still alive gets repopulated and the ordering is
25+
// observable rather than assumed.
26+
func destroyRuntime(t *testing.T) (*Runtime, runners.Proc, string) {
27+
t.Helper()
28+
ctx := context.Background()
29+
30+
runtime := NewRuntime(NewService())
31+
runtime.Location = t.TempDir()
32+
runtime.Logger = runtime.Wool
33+
runtime.Base.Runtime.RuntimeContext = resources.NewRuntimeContextNative()
34+
35+
cache := t.TempDir()
36+
require.NoError(t, os.WriteFile(filepath.Join(cache, "uv.json"), []byte("{}"), 0o600))
37+
runtime.cacheLocation = cache
38+
39+
env, err := runners.NewNativeEnvironment(ctx, t.TempDir())
40+
require.NoError(t, err)
41+
proc, err := env.NewProcess("sh", "-c",
42+
"while true; do touch "+filepath.Join(cache, "heartbeat")+"; sleep 0.05; done")
43+
require.NoError(t, err)
44+
require.NoError(t, proc.Start(ctx))
45+
t.Cleanup(func() { _ = proc.Stop(context.Background()) })
46+
47+
runtime.runner = &runnerHandle{proc: proc, cancel: func() {}}
48+
runtime.runnerEnvironment = env
49+
return runtime, proc, cache
50+
}
51+
52+
func TestDestroyEndsExecutionBeforeClearingCache(t *testing.T) {
53+
ctx := context.Background()
54+
runtime, proc, cache := destroyRuntime(t)
55+
56+
resp, err := runtime.Destroy(ctx, &runtimev0.DestroyRequest{})
57+
require.NoError(t, err)
58+
require.Equal(t, runtimev0.DestroyStatus_SUCCESS, resp.GetStatus().GetState(), resp.GetStatus().GetMessage())
59+
60+
alive, err := proc.IsRunning(ctx)
61+
require.NoError(t, err)
62+
require.False(t, alive, "Destroy must stop the service process without a preceding Stop")
63+
64+
require.Nil(t, currentRunner(runtime))
65+
require.Nil(t, runtime.runnerEnvironment, "the runner environment must be shut down and released")
66+
67+
entries, err := os.ReadDir(cache)
68+
require.NoError(t, err)
69+
require.Empty(t, entries, "the owned cache must be cleared")
70+
71+
// Several heartbeat periods: a process that outlived the wipe repopulates.
72+
time.Sleep(500 * time.Millisecond)
73+
entries, err = os.ReadDir(cache)
74+
require.NoError(t, err)
75+
require.Empty(t, entries, "the cache must be cleared after execution ended, not before")
76+
}
77+
78+
func TestDestroyStopsWatcher(t *testing.T) {
79+
ctx := context.Background()
80+
runtime, _, _ := destroyRuntime(t)
81+
require.NoError(t, os.MkdirAll(filepath.Join(runtime.Location, "code/src"), 0o700))
82+
83+
require.NoError(t, runtime.SetupWatcher(ctx, services.NewWatchConfiguration(requirements), runtime.EventHandler))
84+
events := runtime.Events
85+
86+
_, err := runtime.Destroy(ctx, &runtimev0.DestroyRequest{})
87+
require.NoError(t, err)
88+
89+
select {
90+
case _, open := <-events:
91+
require.False(t, open, "the watcher must be stopped, not still delivering events")
92+
case <-time.After(5 * time.Second):
93+
t.Fatal("watcher still running after Destroy")
94+
}
95+
}
96+
97+
func TestDestroyReportsCacheFailureAfterEndingExecution(t *testing.T) {
98+
if os.Geteuid() == 0 {
99+
t.Skip("root ignores directory permissions, so the wipe cannot be made to fail")
100+
}
101+
ctx := context.Background()
102+
runtime, proc, cache := destroyRuntime(t)
103+
104+
// Removing an entry needs write permission on its parent, so the wipe fails
105+
// while every other resource stays reachable.
106+
require.NoError(t, os.Chmod(cache, 0o500))
107+
t.Cleanup(func() { _ = os.Chmod(cache, 0o700) })
108+
109+
resp, err := runtime.Destroy(ctx, &runtimev0.DestroyRequest{})
110+
require.NoError(t, err)
111+
require.Equal(t, runtimev0.DestroyStatus_ERROR, resp.GetStatus().GetState(),
112+
"a failed cache wipe must be observable, not merely logged")
113+
require.Contains(t, resp.GetStatus().GetMessage(), "cannot remove cache")
114+
115+
alive, err := proc.IsRunning(ctx)
116+
require.NoError(t, err)
117+
require.False(t, alive, "execution cleanup must complete even when the cache wipe fails")
118+
require.Nil(t, currentRunner(runtime))
119+
require.Nil(t, runtime.runnerEnvironment)
120+
}
121+
122+
// stubbornProc fails every Stop, standing in for a process the agent cannot
123+
// reap. Only Stop is ever reached on this path.
124+
type stubbornProc struct{ runners.Proc }
125+
126+
func (stubbornProc) Stop(context.Context) error { return errors.New("process will not die") }
127+
128+
func TestDestroyReportsRunnerFailureAndStillClearsCache(t *testing.T) {
129+
ctx := context.Background()
130+
runtime, _, cache := destroyRuntime(t)
131+
runtime.runner = &runnerHandle{proc: stubbornProc{}, cancel: func() {}}
132+
133+
resp, err := runtime.Destroy(ctx, &runtimev0.DestroyRequest{})
134+
require.NoError(t, err)
135+
require.Equal(t, runtimev0.DestroyStatus_ERROR, resp.GetStatus().GetState(),
136+
"Destroy must not report success after only logging a failed process stop")
137+
require.Contains(t, resp.GetStatus().GetMessage(), "cannot stop the fastapi app")
138+
139+
require.Nil(t, runtime.runnerEnvironment, "a failed stop must not strand the remaining cleanup")
140+
entries, err := os.ReadDir(cache)
141+
require.NoError(t, err)
142+
require.Empty(t, entries)
143+
}
144+
145+
func TestDestroyIsIdempotent(t *testing.T) {
146+
ctx := context.Background()
147+
148+
// Before Init: no runner, no environment, no cache location claimed.
149+
fresh := NewRuntime(NewService())
150+
fresh.Location = t.TempDir()
151+
resp, err := fresh.Destroy(ctx, &runtimev0.DestroyRequest{})
152+
require.NoError(t, err)
153+
require.Equal(t, runtimev0.DestroyStatus_SUCCESS, resp.GetStatus().GetState(), resp.GetStatus().GetMessage())
154+
155+
runtime, _, _ := destroyRuntime(t)
156+
for range 3 {
157+
resp, err = runtime.Destroy(ctx, &runtimev0.DestroyRequest{})
158+
require.NoError(t, err)
159+
require.Equal(t, runtimev0.DestroyStatus_SUCCESS, resp.GetStatus().GetState(), resp.GetStatus().GetMessage())
160+
}
161+
162+
// After Stop, Destroy still succeeds and has nothing left to release.
163+
stopped, _, _ := destroyRuntime(t)
164+
stop, err := stopped.Stop(ctx, &runtimev0.StopRequest{})
165+
require.NoError(t, err)
166+
require.Equal(t, runtimev0.StopStatus_SUCCESS, stop.GetStatus().GetState(), stop.GetStatus().GetMessage())
167+
resp, err = stopped.Destroy(ctx, &runtimev0.DestroyRequest{})
168+
require.NoError(t, err)
169+
require.Equal(t, runtimev0.DestroyStatus_SUCCESS, resp.GetStatus().GetState(), resp.GetStatus().GetMessage())
170+
}
171+
172+
// TestStartAfterDestroyCannotLeaveAReplacement covers the race `codefly`
173+
// shutdown actually runs: a Start already in flight lands after Destroy has
174+
// released the mutex. Destroy releases the execution environment, so that Start
175+
// has nothing to launch into and fails loudly instead of leaving a uvicorn
176+
// running against a wiped cache.
177+
func TestStartAfterDestroyCannotLeaveAReplacement(t *testing.T) {
178+
ctx := context.Background()
179+
runtime, _, _ := destroyRuntime(t)
180+
181+
_, err := runtime.Destroy(ctx, &runtimev0.DestroyRequest{})
182+
require.NoError(t, err)
183+
184+
start, err := runtime.Start(ctx, &runtimev0.StartRequest{})
185+
require.NoError(t, err)
186+
require.Equal(t, runtimev0.StartStatus_ERROR, start.GetStatus().GetState())
187+
require.Nil(t, currentRunner(runtime), "no process may survive Destroy")
188+
}
189+
190+
// TestDestroyStopsRealUvicornWithoutStop is the acceptance case against the
191+
// actual server: a listener that answers requests is gone before Destroy
192+
// reports success, with no Stop anywhere in the path.
193+
func TestDestroyStopsRealUvicornWithoutStop(t *testing.T) {
194+
ctx := context.Background()
195+
source := uvProject(t, servingApp)
196+
runtime := uvicornRuntime(t, source, false)
197+
runtime.cacheLocation = t.TempDir()
198+
199+
_, err := runtime.Start(ctx, &runtimev0.StartRequest{})
200+
require.NoError(t, err)
201+
require.Eventually(t, func() bool { return serves(runtime.port) }, 30*time.Second, 200*time.Millisecond,
202+
"the fixture must be serving before Destroy means anything")
203+
204+
handle := currentRunner(runtime)
205+
resp, err := runtime.Destroy(ctx, &runtimev0.DestroyRequest{})
206+
require.NoError(t, err)
207+
require.Equal(t, runtimev0.DestroyStatus_SUCCESS, resp.GetStatus().GetState(), resp.GetStatus().GetMessage())
208+
209+
running, err := handle.proc.IsRunning(ctx)
210+
require.NoError(t, err)
211+
require.False(t, running, "uvicorn must be gone before Destroy reports success")
212+
require.False(t, serves(runtime.port), "the listener must be gone before Destroy reports success")
213+
}
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+
}

0 commit comments

Comments
 (0)