Skip to content

Commit 4f6c187

Browse files
antoinetoussaint-byteclaude
antoinetoussaint-byte
and
claude
authored
fix: serialize Init against Start (#24) (#26)
* fix: serialize Init against Start (#24) Init publishes the runtime context, the network mappings, port, grpcPort and the environment-variable entries that Start consumes, all outside any lock, while Start reads them under runnerMu. An overlapping Start could launch uvicorn on a half-written port, and go test -race flags the pair. Guarding only port and grpcPort would leave the environment manager racing: resources.EnvironmentVariableManager has no synchronization of its own, so the two calls are serialized outright, under a lock of their own so Stop and Destroy can still interrupt Init's runner section. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix: decide restarts on what was launched, not just the request (#24) Start answered "nothing to do" whenever the StartRequest matched the one the running process was launched with. The request does not carry this service's own port: s.port and s.grpcPort come from the InitRequest's proposed network mappings. An Init that republished them between two identical Starts therefore left uvicorn bound to the superseded port while the agent answered STARTED, so the orchestrator routed to a port nothing was listening on and the agent reported healthy throughout. Folding the ports into startInputs would have been wrong: that fingerprint also gates applyStartInputs, which must fire only when the request itself changed, because the environment manager appends overrides and endpoints and never removes them. The two questions are distinct, so they now have distinct fingerprints — startInputs for "did the request change", launchInputs for "is the running process still the one this request describes". launchInputs covers every input to the command line, hot-reload included, not just the ports this was found through. Start also acted on requests its caller had abandoned. It can wait on initMu for as long as an Init takes, and the process it launches runs on a background context, so a Start acquiring the lock after its deadline passed would leave a service running for a request nobody awaited. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test: make the Init/Start race test detect the race (#24) TestInitIsSafeWithConcurrentStart released Init and Start from a bare start gate. Measured against code with initMu removed, it reported the race 0 times in 10: Init takes runnerMu three times (destroyed, SetRuntimeContext, runnerEnv), and each acquisition orders the two bodies against Start's own runnerMu section, so the detector sees no concurrency. Its one postcondition — that a uvicorn process exists — also held in the interleaving where Start ran first and launched on port 0. #25 had already retired this shape for the teardown test; this repeats its fix, gating on the window Init is vulnerable in and repeating so the detection is reliable rather than sampled. The suite never exercised a cold Init, because a pre-installed environment sends Init down runnerEnv's already-published branch and skips CreateRunnerEnvironment — where the network mappings and the whole environment manager are read and ActiveEnv is published. That was untestable while #23 was open and is not any more, so the cold path gets its own overlap, with Start pointed at a missing source location so it reads everything and then fails in exec instead of launching a real uvicorn. Both tests now detect the race 5/5 with initMu removed. The 100ms sleep now fails fast: a Start that completes while Init is still gated is the defect, so it is observed and reported rather than waited out. It is deliberately only observed on that side — receiving from the channel on the passing path would order Start ahead of Init's release and hide the race. Reuses initTestRuntime rather than carrying a second copy of the same fixture, and stops indexing past --port blindly. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent da01e1f commit 4f6c187

3 files changed

Lines changed: 345 additions & 7 deletions

File tree

‎init_start_test.go‎

Lines changed: 193 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,193 @@
1+
package main
2+
3+
import (
4+
"context"
5+
"os"
6+
"path/filepath"
7+
"slices"
8+
"sync"
9+
"testing"
10+
"time"
11+
12+
basev0 "github.com/codefly-dev/core/generated/go/codefly/base/v0"
13+
runtimev0 "github.com/codefly-dev/core/generated/go/codefly/services/runtime/v0"
14+
"github.com/stretchr/testify/require"
15+
)
16+
17+
// initStartTestRuntime builds on initTestRuntime and adds what an Init/Start
18+
// overlap needs: a runner environment Start can launch against without a real
19+
// uv, a cache location for the dependency hashes, and a configuration, so Init
20+
// still has environment-manager work to do after its runner section — the
21+
// window a Start can land in.
22+
func initStartTestRuntime(t *testing.T) (*Runtime, *fakeRunnerEnvironment, *runtimev0.InitRequest) {
23+
t.Helper()
24+
25+
runtime, req := initTestRuntime(t)
26+
runtime.cacheLocation = t.TempDir()
27+
env := &fakeRunnerEnvironment{}
28+
runtime.runnerEnvironment = env
29+
req.Configuration = &basev0.Configuration{
30+
Origin: "mod/probe",
31+
Infos: []*basev0.ConfigurationInformation{{
32+
Name: "postgres",
33+
ConfigurationValues: []*basev0.ConfigurationValue{{Key: "url", Value: "postgres://localhost"}},
34+
}},
35+
}
36+
return runtime, env, req
37+
}
38+
39+
// uvicornProc returns the process Start launched, told apart from the ones Init
40+
// runs on the same environment by the binary it invokes.
41+
func uvicornProc(t *testing.T, env *fakeRunnerEnvironment) *fakeProc {
42+
t.Helper()
43+
for _, proc := range env.started() {
44+
if slices.Contains(proc.args, "uvicorn") {
45+
return proc
46+
}
47+
}
48+
t.Fatal("Start never launched uvicorn")
49+
return nil
50+
}
51+
52+
// uvicornPort returns the port Start bound the process to.
53+
func uvicornPort(t *testing.T, env *fakeRunnerEnvironment) string {
54+
t.Helper()
55+
args := uvicornProc(t, env).args
56+
i := slices.Index(args, "--port")
57+
require.GreaterOrEqual(t, i, 0, "uvicorn was launched without a port: %v", args)
58+
require.Less(t, i+1, len(args), "uvicorn was launched with a bare --port: %v", args)
59+
return args[i+1]
60+
}
61+
62+
// gatingEnv holds Init inside its runner section until the test releases it, so
63+
// a Start can be issued while Init is demonstrably mid-flight.
64+
type gatingEnv struct {
65+
*fakeRunnerEnvironment
66+
entered chan struct{}
67+
release chan struct{}
68+
}
69+
70+
func (e *gatingEnv) Init(context.Context) error {
71+
close(e.entered)
72+
<-e.release
73+
return nil
74+
}
75+
76+
// TestStartWaitsForInitToPublishItsPorts is the contract: Init publishes the
77+
// port Start binds uvicorn to, so a Start landing in the middle of an Init runs
78+
// after it and launches on the published port rather than on whatever the field
79+
// happened to hold.
80+
//
81+
// It is also where `-race` sees the two calls overlap on the state Init
82+
// publishes. Gating Init inside its runner section is what makes that reliable:
83+
// released from a bare start gate the two bodies order themselves through
84+
// runnerMu, and the detector reports nothing at all.
85+
func TestStartWaitsForInitToPublishItsPorts(t *testing.T) {
86+
for range 20 {
87+
runInitAgainstStart(t)
88+
}
89+
}
90+
91+
func runInitAgainstStart(t *testing.T) {
92+
t.Helper()
93+
runtime, fake, req := initStartTestRuntime(t)
94+
env := &gatingEnv{fakeRunnerEnvironment: fake, entered: make(chan struct{}), release: make(chan struct{})}
95+
runtime.runnerEnvironment = env
96+
97+
var wg sync.WaitGroup
98+
var initResp *runtimev0.InitResponse
99+
var initErr error
100+
wg.Add(1)
101+
go func() {
102+
defer wg.Done()
103+
initResp, initErr = runtime.Init(context.Background(), req)
104+
}()
105+
106+
<-env.entered
107+
108+
startDone := make(chan struct{})
109+
var startResp *runtimev0.StartResponse
110+
var startErr error
111+
wg.Add(1)
112+
go func() {
113+
defer wg.Done()
114+
startResp, startErr = runtime.Start(context.Background(), &runtimev0.StartRequest{})
115+
close(startDone)
116+
}()
117+
118+
// Nothing observable says the Start goroutine reached the lock, so give it
119+
// the room an unserialized Start would need. Observing startDone is only
120+
// safe on the failing side: receiving from it would order the Start
121+
// goroutine ahead of the release below and hide the very race this test
122+
// exists to surface, so the good path must fall through the timeout having
123+
// synchronized with nothing.
124+
select {
125+
case <-startDone:
126+
t.Fatal("Start ran to completion while Init was still inside its runner section")
127+
case <-time.After(50 * time.Millisecond):
128+
}
129+
close(env.release)
130+
wg.Wait()
131+
132+
require.NoError(t, initErr)
133+
require.Equal(t, runtimev0.InitStatus_READY, initResp.GetStatus().GetState(), initResp.GetStatus().GetMessage())
134+
require.NoError(t, startErr)
135+
require.Equal(t, runtimev0.StartStatus_STARTED, startResp.GetStatus().GetState(), startResp.GetStatus().GetMessage())
136+
require.Equal(t, "9999", uvicornPort(t, fake))
137+
}
138+
139+
// TestInitIsSafeWithConcurrentStartOnAColdRuntime covers the path the gated
140+
// test cannot reach: the first Init, which runs CreateRunnerEnvironment. That
141+
// function reads the network mappings and the whole environment manager, and
142+
// publishes ActiveEnv — none of it exercised once a runner environment is
143+
// already in place.
144+
//
145+
// Start is pointed at a source location that does not exist, so it does all of
146+
// its reading and then fails in exec rather than launching a real uvicorn.
147+
func TestInitIsSafeWithConcurrentStartOnAColdRuntime(t *testing.T) {
148+
isolateWorkingDir(t)
149+
for range 20 {
150+
runColdInitAgainstStart(t)
151+
}
152+
}
153+
154+
func runColdInitAgainstStart(t *testing.T) {
155+
t.Helper()
156+
runtime, req := initTestRuntime(t)
157+
// Init creates and publishes the runner environment — the section under
158+
// test — and then stops on the missing project file, well before uv.
159+
require.NoError(t, os.Remove(filepath.Join(runtime.Service.SourceLocation, "pyproject.toml")))
160+
runtime.Service.SourceLocation = filepath.Join(runtime.Service.SourceLocation, "does-not-exist")
161+
162+
var initResp *runtimev0.InitResponse
163+
var initErr error
164+
var startResp *runtimev0.StartResponse
165+
166+
gate := make(chan struct{})
167+
var wg sync.WaitGroup
168+
wg.Add(2)
169+
go func() {
170+
defer wg.Done()
171+
<-gate
172+
initResp, initErr = runtime.Init(context.Background(), req)
173+
}()
174+
go func() {
175+
defer wg.Done()
176+
<-gate
177+
// Land in the window Init is vulnerable in. Before the environment is
178+
// published Start refuses at once, having read none of the state Init
179+
// is writing, and the accesses that would race never execute.
180+
waitForRunnerEnvironment(runtime)
181+
startResp, _ = runtime.Start(context.Background(), &runtimev0.StartRequest{})
182+
}()
183+
close(gate)
184+
wg.Wait()
185+
186+
require.NoError(t, initErr)
187+
require.NotNil(t, initResp, "Init must not panic when a Start lands mid-flight")
188+
require.Contains(t, initResp.GetStatus().GetMessage(), "no pyproject.toml",
189+
"Init must run past its runner section, or this tests nothing")
190+
require.NotNil(t, startResp, "Start must not panic against a concurrent Init")
191+
require.DirExists(t, filepath.Join(runtime.Location, ".cache/local"),
192+
"Init must have created and published the runner environment")
193+
}

‎runtime.go‎

Lines changed: 84 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -73,15 +73,55 @@ type Runtime struct {
7373
// runnerCreateMu single-flights the creation of the runner environment.
7474
// runnerMu cannot do that job: publishing is cheap but building is a
7575
// docker pull, and holding runnerMu across it would park every Stop.
76-
// Without it two concurrent Inits each build an environment, one gets
77-
// published and the other is left initialized — holding a docker client
78-
// and a log stream — with nothing referencing it to shut it down.
76+
// Without it two callers each build an environment, one gets published and
77+
// the other is left initialized — holding a docker client and a log stream
78+
// — with nothing referencing it to shut it down.
79+
//
80+
// Init no longer reaches it concurrently: initMu below serializes Init
81+
// against itself, so runnerEnv's only production caller is single-threaded.
82+
// It stays because it guards runnerEnv rather than Init — the invariant
83+
// belongs to the function that publishes the field, not to the one caller
84+
// that happens to hold a bigger lock today.
7985
runnerCreateMu sync.Mutex
8086

87+
// initMu serializes Init against Start. Init publishes what Start consumes
88+
// — the network mappings, port and grpcPort, and the entries it folds into
89+
// EnvironmentVariables — none of which runnerMu covers, so an overlapping
90+
// Start can otherwise launch uvicorn on a half-written port, or read the
91+
// environment manager while Init is appending to it. The manager is core's
92+
// resources.EnvironmentVariableManager and carries no synchronization of
93+
// its own, so nothing finer than serializing the two calls covers it.
94+
//
95+
// It is a lock of its own rather than runnerMu because Init's runner
96+
// section can pull an image or run `uv sync`: Stop and Destroy take only
97+
// runnerMu, so a teardown is never queued behind that, and Init carries the
98+
// environment it captured through it. Start has nothing to run against
99+
// until Init has published its ports, so Start is the side that waits.
100+
// Holding it for the whole of Init also serializes Init against itself.
101+
//
102+
// LOCK ORDER for all three mutexes on this struct:
103+
//
104+
// initMu -> runnerCreateMu -> runnerMu
105+
//
106+
// Every acquisition follows it and nothing takes them the other way around.
107+
// The trap is that runnerMu is the innermost: a helper that already holds
108+
// it must not call anything reaching for the other two, and in particular
109+
// must not call CreateRunnerEnvironment, which takes runnerMu itself to
110+
// publish. Callers reach that through runnerEnv, holding none of the three.
111+
initMu sync.Mutex
112+
81113
// startInputs renders the StartRequest fields the running process was
82114
// launched with, so a later Start can tell whether they still match.
83115
startInputs string
84116

117+
// launchInputs renders everything that shapes the launched process: the
118+
// StartRequest fields above plus the agent-owned argv state Init publishes.
119+
// It answers a different question from startInputs — "is the process that
120+
// is running still the one this request describes", rather than "did the
121+
// request change" — and the two diverge whenever an Init republishes the
122+
// ports without the StartRequest changing.
123+
launchInputs string
124+
85125
// appliedOverrides counts, per key, how many override entries this agent
86126
// has handed to the environment manager. The manager appends overrides and
87127
// never removes them, so the counts are what lets a withdrawn key be
@@ -175,6 +215,11 @@ func (s *Runtime) resolveRuntimeImage() (*resources.DockerImage, error) {
175215
// CreateRunnerEnvironment builds the execution environment and the cache
176216
// location that belongs to it, publishes both under runnerMu and returns them
177217
// so the caller can keep working against locals.
218+
//
219+
// It must be called with runnerMu NOT held — it takes that lock to publish —
220+
// and through runnerEnv, which single-flights it under runnerCreateMu. Calling
221+
// it directly from a path already holding runnerMu deadlocks; calling it
222+
// directly from two goroutines builds two environments and strands one.
178223
func (s *Runtime) CreateRunnerEnvironment(ctx context.Context) (runners.RunnerEnvironment, string, error) {
179224
s.Wool.Debug("creating runner environment in", wool.DirField(s.Identity.WorkspacePath))
180225
image, err := s.resolveRuntimeImage()
@@ -330,6 +375,9 @@ func (s *Runtime) Init(ctx context.Context, req *runtimev0.InitRequest) (*runtim
330375

331376
s.Base.Runtime.LogInitRequest(req)
332377

378+
s.initMu.Lock()
379+
defer s.initMu.Unlock()
380+
333381
// Init opens a new lifecycle, so a runtime torn down earlier accepts work
334382
// again — otherwise Init would succeed and every later Start refuse.
335383
s.runnerMu.Lock()
@@ -448,6 +496,17 @@ func (s *Runtime) Start(ctx context.Context, req *runtimev0.StartRequest) (*runt
448496
defer s.Wool.Catch()
449497
ctx = s.Wool.Inject(ctx)
450498

499+
s.initMu.Lock()
500+
defer s.initMu.Unlock()
501+
502+
// initMu is not cancellable, and Init can hold it across a docker pull and
503+
// a `uv sync`. The process launched below outlives this RPC on a context of
504+
// its own, so a Start that only reaches here after its caller gave up would
505+
// leave a service running for a request nobody is waiting on.
506+
if err := ctx.Err(); err != nil {
507+
return s.Base.Runtime.StartError(err)
508+
}
509+
451510
s.runnerMu.Lock()
452511
defer s.runnerMu.Unlock()
453512

@@ -458,6 +517,7 @@ func (s *Runtime) Start(ctx context.Context, req *runtimev0.StartRequest) (*runt
458517
}
459518

460519
inputs := startInputs(req)
520+
launch := launchInputs(inputs, s.port, s.grpcPort, s.FastAPI.Settings.HotReload)
461521

462522
if s.runner != nil {
463523
switch {
@@ -466,10 +526,13 @@ func (s *Runtime) Start(ctx context.Context, req *runtimev0.StartRequest) (*runt
466526
// again is the chance to bring the service back, not something to
467527
// answer from a stale handle.
468528
s.Infof("fastapi app is no longer running, starting it again")
469-
case inputs == s.startInputs:
470-
// A running process keeps the environment it was launched with and
471-
// uvicorn --reload already covers source edits, so an unchanged
472-
// request describes exactly what is running: nothing to do.
529+
case launch == s.launchInputs:
530+
// A running process keeps the environment AND the command line it
531+
// was launched with, and uvicorn --reload already covers source
532+
// edits, so nothing to do. The request alone cannot answer this:
533+
// an Init between two identical Starts can republish the ports,
534+
// and the process bound to the old one is not what this request
535+
// describes however unchanged the request is.
473536
return s.Base.Runtime.StartResponse()
474537
default:
475538
// Different inputs only reach the service through a new process:
@@ -529,6 +592,7 @@ func (s *Runtime) Start(ctx context.Context, req *runtimev0.StartRequest) (*runt
529592

530593
s.runner = handle
531594
s.startInputs = inputs
595+
s.launchInputs = launch
532596

533597
if s.FastAPI.Settings.HotReload {
534598
conf := services.NewWatchConfiguration(requirements)
@@ -722,6 +786,19 @@ func startInputs(req *runtimev0.StartRequest) string {
722786
return strings.Join(append(lines, dependencies...), "\n")
723787
}
724788

789+
// launchInputs extends the StartRequest fingerprint with the state this agent
790+
// owns rather than receives: the ports Init resolves from the proposed network
791+
// mappings, and the hot-reload setting. Both reach uvicorn's command line, so a
792+
// process launched under different values is not serving what the caller asked
793+
// for even when the StartRequest is byte-identical.
794+
//
795+
// It is deliberately separate from startInputs: that one gates applyStartInputs,
796+
// which must fire only when the request itself changed, because the environment
797+
// manager appends overrides and endpoints without ever removing any.
798+
func launchInputs(requestInputs string, port uint16, grpcPort uint16, hotReload bool) string {
799+
return fmt.Sprintf("%s\nport %d\ngrpc-port %d\nhot-reload %t", requestInputs, port, grpcPort, hotReload)
800+
}
801+
725802
// applyStartInputs folds the StartRequest into the environment manager. Callers
726803
// must only invoke it when the inputs actually changed: the manager appends
727804
// overrides and endpoints without ever removing any, so re-folding an unchanged

0 commit comments

Comments
 (0)