Skip to content

Commit 5df1548

Browse files
fix: give Init a locking discipline for the runner state (#23) (#25)
* fix: give Init a locking discipline for the runner state (#23) Init read and published s.runnerEnvironment and s.cacheLocation with no lock while Stop and Destroy touch the same fields under runnerMu. CreateRunnerEnvironment now builds the environment and its cache location into locals and publishes both under runnerMu, returning them so Init works against the pair it captured rather than re-reading fields a concurrent teardown can replace mid-flight. Destroy reads cacheLocation and the runtime context inside the critical section it already takes, and SetRuntimeContext publishes under the same lock. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix: enforce the runner-state locking that #23 introduced (#23) Review of the previous commit turned up three defects in the fix itself. CI ran the shared `go test ./...` with no race detector, so TestInitIsSafeWithConcurrentTeardown passed on code with the lock removed — the guard the issue asked for was inert. Add a race job; the shared workflow exposes no input for it. The test also raced the two calls from a bare start gate, which the teardown always won: it found no environment, touched none of the runner state, and the accesses that would race never executed. It now waits for Init to publish before tearing down, and repeats, so detection is reliable rather than one-in-three. runnerEnv checked the field and created outside the same lock — the defect class this fix exists to remove, one level up. Two concurrent Inits each built an environment; docker's GetContainer is name-idempotent so they converged on one container, but on two DockerEnvironment objects, and the loser kept a docker client and a log stream that nothing could shut down. Single-flight the creation on its own mutex, so runnerMu is still held only for the read and the publish and never across a docker pull. Stop shut the environment down and left the field pointing at it. Shutdown closes the docker client, so the next Init was handed a corpse — and runnerEnv's contract quietly assumed otherwise. Release it, and the handle Code and the REPL hold, on a successful shutdown only. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs: point the ActiveEnv comment at the issue it tracks (#23) The comment was written before the issue was filed and named #26; the reader-side race is tracked in #27. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent eb917be commit 5df1548

3 files changed

Lines changed: 382 additions & 39 deletions

File tree

‎.github/workflows/ci.yml‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,3 +19,25 @@ jobs:
1919
setup-run: |
2020
curl -LsSf https://astral.sh/uv/install.sh | sh
2121
echo "$HOME/.local/bin" >> "$GITHUB_PATH"
22+
23+
# The runner lifecycle (Init / Start / Stop / Destroy) is concurrent gRPC
24+
# handlers sharing runnerMu, and its tests assert through the race detector:
25+
# a lost lock is a data race, not a failed assertion, so the shared workflow's
26+
# plain `go test` passes on code with the lock removed. This job is what makes
27+
# that regression visible; the shared workflow has no input to enable -race.
28+
race:
29+
runs-on: ubuntu-latest
30+
steps:
31+
- uses: actions/checkout@v4
32+
- uses: actions/setup-go@v5
33+
with:
34+
go-version-file: go.mod
35+
# Same reason as the shared workflow: setup-go's cache tar corrupts
36+
# the downloaded toolchain module on restore.
37+
cache: false
38+
- name: Setup
39+
run: |
40+
curl -LsSf https://astral.sh/uv/install.sh | sh
41+
echo "$HOME/.local/bin" >> "$GITHUB_PATH"
42+
- name: Test (race detector)
43+
run: go test -race ./...

‎init_test.go‎

Lines changed: 265 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,265 @@
1+
package main
2+
3+
import (
4+
"context"
5+
"os"
6+
"path/filepath"
7+
"sync"
8+
"testing"
9+
"time"
10+
11+
basev0 "github.com/codefly-dev/core/generated/go/codefly/base/v0"
12+
runtimev0 "github.com/codefly-dev/core/generated/go/codefly/services/runtime/v0"
13+
"github.com/codefly-dev/core/resources"
14+
runners "github.com/codefly-dev/core/runners/base"
15+
"github.com/codefly-dev/core/standards"
16+
"github.com/stretchr/testify/require"
17+
)
18+
19+
const probeProject = `[project]
20+
name = "init-probe"
21+
version = "0.0.0"
22+
`
23+
24+
// isolateWorkingDir moves the test off the checkout. builders.Dependencies
25+
// resolves its hash file relative to the cache location, so a regression that
26+
// empties that location writes uv.hash into the working directory.
27+
func isolateWorkingDir(t *testing.T) {
28+
t.Helper()
29+
t.Chdir(t.TempDir())
30+
}
31+
32+
// initTestRuntime stands a Runtime up at the point Init is called from: a
33+
// native runtime, a source tree holding the files Init hashes, and the REST
34+
// endpoint the proposed mappings resolve.
35+
func initTestRuntime(t *testing.T) (*Runtime, *runtimev0.InitRequest) {
36+
t.Helper()
37+
38+
runtime := NewRuntime(NewService())
39+
runtime.Location = t.TempDir()
40+
runtime.Logger = runtime.Wool
41+
// Load is what normally supplies these; these tests call Init directly.
42+
runtime.Identity = resources.ServiceIdentityFromProto(&basev0.ServiceIdentity{
43+
Name: "probe",
44+
Module: "mod",
45+
Workspace: "workspace",
46+
WorkspacePath: runtime.Location,
47+
})
48+
runtime.Base.Runtime.RuntimeContext = resources.NewRuntimeContextNative()
49+
50+
source := t.TempDir()
51+
require.NoError(t, os.MkdirAll(filepath.Join(source, "src"), 0o700))
52+
require.NoError(t, os.WriteFile(filepath.Join(source, "pyproject.toml"), []byte(probeProject), 0o600))
53+
require.NoError(t, os.WriteFile(filepath.Join(source, "src", "main.py"), []byte("app = None\n"), 0o600))
54+
runtime.Service.SourceLocation = source
55+
56+
endpoint := &basev0.Endpoint{Module: "mod", Service: "probe", Name: "rest", Api: standards.REST}
57+
runtime.FastAPI.RestEndpoint = endpoint
58+
t.Cleanup(runtime.Base.StopWatcher)
59+
60+
return runtime, &runtimev0.InitRequest{
61+
RuntimeContext: resources.NewRuntimeContextNative(),
62+
ProposedNetworkMappings: []*basev0.NetworkMapping{{
63+
Endpoint: endpoint,
64+
Instances: []*basev0.NetworkInstance{{
65+
Access: resources.NewNativeNetworkAccess(),
66+
Hostname: "localhost",
67+
Port: 9999,
68+
Address: "http://localhost:9999",
69+
}},
70+
}},
71+
}
72+
}
73+
74+
// releasingEnv releases the runtime's runner state the moment Init hands it
75+
// control, standing in for a Stop or Destroy landing in the middle of Init's
76+
// runner section.
77+
type releasingEnv struct {
78+
*fakeRunnerEnvironment
79+
runtime *Runtime
80+
}
81+
82+
func (e *releasingEnv) Init(context.Context) error {
83+
e.runtime.runnerMu.Lock()
84+
defer e.runtime.runnerMu.Unlock()
85+
e.runtime.runnerEnvironment = nil
86+
e.runtime.cacheLocation = ""
87+
return nil
88+
}
89+
90+
// TestInitWorksAgainstTheEnvironmentItCaptured is the contract: Init reads the
91+
// environment and its cache location once and then works against those, so a
92+
// teardown replacing the fields underneath it cannot leave Init dereferencing
93+
// a nil environment or caching against nothing.
94+
func TestInitWorksAgainstTheEnvironmentItCaptured(t *testing.T) {
95+
isolateWorkingDir(t)
96+
ctx := context.Background()
97+
runtime, req := initTestRuntime(t)
98+
99+
env := &releasingEnv{fakeRunnerEnvironment: &fakeRunnerEnvironment{}, runtime: runtime}
100+
cache := t.TempDir()
101+
runtime.runnerEnvironment = env
102+
runtime.cacheLocation = cache
103+
104+
resp, err := runtime.Init(ctx, req)
105+
require.NoError(t, err)
106+
require.Equal(t, runtimev0.InitStatus_READY, resp.GetStatus().GetState(), resp.GetStatus().GetMessage())
107+
108+
var commands [][]string
109+
for _, proc := range env.started() {
110+
commands = append(commands, append([]string{proc.bin}, proc.args...))
111+
}
112+
require.Equal(t, [][]string{
113+
{"uv", "sync"},
114+
{"uv", "run", "python", "src/openapi.py"},
115+
}, commands, "both must run on the captured environment")
116+
117+
entries, err := os.ReadDir(cache)
118+
require.NoError(t, err)
119+
require.NotEmpty(t, entries, "the dependency hashes must land in the captured cache location")
120+
}
121+
122+
// TestInitIsSafeWithConcurrentTeardown overlaps Init with the calls that own
123+
// the same fields. Under `-race` this is what catches Init publishing or
124+
// reading the runner state outside runnerMu; without it, it still pins that
125+
// neither side panics however they interleave.
126+
func TestInitIsSafeWithConcurrentTeardown(t *testing.T) {
127+
for _, tc := range []struct {
128+
name string
129+
// teardown reports whether the call returned a response: a panic is
130+
// swallowed by the deferred Wool.Catch and surfaces as a nil one.
131+
teardown func(*Runtime) bool
132+
}{
133+
{"destroy", func(r *Runtime) bool {
134+
resp, err := r.Destroy(context.Background(), &runtimev0.DestroyRequest{})
135+
return err == nil && resp != nil
136+
}},
137+
{"stop", func(r *Runtime) bool {
138+
resp, err := r.Stop(context.Background(), &runtimev0.StopRequest{})
139+
return err == nil && resp != nil
140+
}},
141+
} {
142+
t.Run(tc.name, func(t *testing.T) {
143+
isolateWorkingDir(t)
144+
// One interleaving proves little: the race detector reports only
145+
// accesses that actually execute, and a teardown landing before
146+
// Init publishes touches none of the runner state. Repeat so both
147+
// orders occur.
148+
for range 50 {
149+
runInitAgainstTeardown(t, tc.teardown)
150+
}
151+
})
152+
}
153+
}
154+
155+
func runInitAgainstTeardown(t *testing.T, teardown func(*Runtime) bool) {
156+
t.Helper()
157+
runtime, req := initTestRuntime(t)
158+
// Init creates and publishes the runner environment — the section under
159+
// test — and then stops on the missing project file, well before it would
160+
// reach for uv.
161+
require.NoError(t, os.Remove(filepath.Join(runtime.Service.SourceLocation, "pyproject.toml")))
162+
163+
var initResp *runtimev0.InitResponse
164+
var initErr error
165+
var teardownOK bool
166+
167+
gate := make(chan struct{})
168+
var wg sync.WaitGroup
169+
wg.Add(2)
170+
go func() {
171+
defer wg.Done()
172+
<-gate
173+
initResp, initErr = runtime.Init(context.Background(), req)
174+
}()
175+
go func() {
176+
defer wg.Done()
177+
<-gate
178+
// Land in the window Init is actually vulnerable in. Racing the two
179+
// from a bare start gate is not enough: the teardown wins, finds no
180+
// environment, and touches none of the runner state — so the accesses
181+
// that would race never execute and the detector sees nothing.
182+
waitForRunnerEnvironment(runtime)
183+
teardownOK = teardown(runtime)
184+
}()
185+
close(gate)
186+
wg.Wait()
187+
188+
require.NoError(t, initErr)
189+
require.NotNil(t, initResp, "Init must not panic when a teardown lands mid-flight")
190+
require.Equal(t, runtimev0.InitStatus_ERROR, initResp.GetStatus().GetState())
191+
require.Contains(t, initResp.GetStatus().GetMessage(), "no pyproject.toml",
192+
"Init must run past its runner section, or this tests nothing")
193+
require.True(t, teardownOK, "the teardown must not panic against a concurrent Init")
194+
require.DirExists(t, filepath.Join(runtime.Location, ".cache/local"),
195+
"Init must have created and published the runner environment")
196+
}
197+
198+
// waitForRunnerEnvironment blocks until Init has published the environment.
199+
// The read is taken under runnerMu, so waiting introduces no edge of its own
200+
// between Init and the teardown.
201+
func waitForRunnerEnvironment(runtime *Runtime) {
202+
for range 2000 {
203+
runtime.runnerMu.Lock()
204+
published := runtime.runnerEnvironment != nil
205+
runtime.runnerMu.Unlock()
206+
if published {
207+
return
208+
}
209+
time.Sleep(time.Millisecond)
210+
}
211+
}
212+
213+
// TestRunnerEnvCreatesOneEnvironment pins the single-flight: concurrent Inits
214+
// on a cold runtime must share one environment. Without it each caller builds
215+
// its own, one wins the field, and the losers are left initialized — holding a
216+
// docker client and a log stream — with nothing left to shut them down.
217+
func TestRunnerEnvCreatesOneEnvironment(t *testing.T) {
218+
isolateWorkingDir(t)
219+
runtime, _ := initTestRuntime(t)
220+
221+
const callers = 8
222+
envs := make([]runners.RunnerEnvironment, callers)
223+
errs := make([]error, callers)
224+
225+
gate := make(chan struct{})
226+
var wg sync.WaitGroup
227+
for i := range callers {
228+
wg.Add(1)
229+
go func() {
230+
defer wg.Done()
231+
<-gate
232+
envs[i], _, errs[i] = runtime.runnerEnv(context.Background())
233+
}()
234+
}
235+
close(gate)
236+
wg.Wait()
237+
238+
for i := range callers {
239+
require.NoError(t, errs[i])
240+
require.NotNil(t, envs[i])
241+
require.True(t, envs[i] == envs[0], "every caller must get the one environment that was published")
242+
}
243+
}
244+
245+
// TestInitAfterStopBuildsAFreshEnvironment pins what Stop owes the next Init:
246+
// Shutdown closes the docker client, so an environment left in the field after
247+
// it cannot serve another process.
248+
func TestInitAfterStopBuildsAFreshEnvironment(t *testing.T) {
249+
isolateWorkingDir(t)
250+
ctx := context.Background()
251+
runtime, _ := initTestRuntime(t)
252+
253+
first, _, err := runtime.runnerEnv(ctx)
254+
require.NoError(t, err)
255+
require.NotNil(t, first)
256+
257+
resp, err := runtime.Stop(ctx, &runtimev0.StopRequest{})
258+
require.NoError(t, err)
259+
require.Equal(t, runtimev0.StopStatus_SUCCESS, resp.GetStatus().GetState(), resp.GetStatus().GetMessage())
260+
require.Nil(t, runtime.FastAPI.Service.ActiveEnv, "Code and the REPL must not be left holding a dead environment")
261+
262+
second, _, err := runtime.runnerEnv(ctx)
263+
require.NoError(t, err)
264+
require.False(t, second == first, "Stop shut the environment down; Init must not be handed the corpse")
265+
}

0 commit comments

Comments
 (0)