Skip to content

Commit ea387a8

Browse files
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>
1 parent e4757ce commit ea387a8

2 files changed

Lines changed: 130 additions & 9 deletions

File tree

‎runtime.go‎

Lines changed: 62 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -73,9 +73,15 @@ 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

8187
// initMu serializes Init against Start. Init publishes what Start consumes
@@ -91,15 +97,31 @@ type Runtime struct {
9197
// runnerMu, so a teardown is never queued behind that, and Init carries the
9298
// environment it captured through it. Start has nothing to run against
9399
// 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
94105
//
95-
// Lock order is initMu, then runnerCreateMu, then runnerMu; nothing takes
96-
// them the other way around.
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.
97111
initMu sync.Mutex
98112

99113
// startInputs renders the StartRequest fields the running process was
100114
// launched with, so a later Start can tell whether they still match.
101115
startInputs string
102116

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+
103125
// appliedOverrides counts, per key, how many override entries this agent
104126
// has handed to the environment manager. The manager appends overrides and
105127
// never removes them, so the counts are what lets a withdrawn key be
@@ -193,6 +215,11 @@ func (s *Runtime) resolveRuntimeImage() (*resources.DockerImage, error) {
193215
// CreateRunnerEnvironment builds the execution environment and the cache
194216
// location that belongs to it, publishes both under runnerMu and returns them
195217
// 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.
196223
func (s *Runtime) CreateRunnerEnvironment(ctx context.Context) (runners.RunnerEnvironment, string, error) {
197224
s.Wool.Debug("creating runner environment in", wool.DirField(s.Identity.WorkspacePath))
198225
image, err := s.resolveRuntimeImage()
@@ -472,6 +499,14 @@ func (s *Runtime) Start(ctx context.Context, req *runtimev0.StartRequest) (*runt
472499
s.initMu.Lock()
473500
defer s.initMu.Unlock()
474501

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+
475510
s.runnerMu.Lock()
476511
defer s.runnerMu.Unlock()
477512

@@ -482,6 +517,7 @@ func (s *Runtime) Start(ctx context.Context, req *runtimev0.StartRequest) (*runt
482517
}
483518

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

486522
if s.runner != nil {
487523
switch {
@@ -490,10 +526,13 @@ func (s *Runtime) Start(ctx context.Context, req *runtimev0.StartRequest) (*runt
490526
// again is the chance to bring the service back, not something to
491527
// answer from a stale handle.
492528
s.Infof("fastapi app is no longer running, starting it again")
493-
case inputs == s.startInputs:
494-
// A running process keeps the environment it was launched with and
495-
// uvicorn --reload already covers source edits, so an unchanged
496-
// 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.
497536
return s.Base.Runtime.StartResponse()
498537
default:
499538
// Different inputs only reach the service through a new process:
@@ -553,6 +592,7 @@ func (s *Runtime) Start(ctx context.Context, req *runtimev0.StartRequest) (*runt
553592

554593
s.runner = handle
555594
s.startInputs = inputs
595+
s.launchInputs = launch
556596

557597
if s.FastAPI.Settings.HotReload {
558598
conf := services.NewWatchConfiguration(requirements)
@@ -746,6 +786,19 @@ func startInputs(req *runtimev0.StartRequest) string {
746786
return strings.Join(append(lines, dependencies...), "\n")
747787
}
748788

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+
749802
// applyStartInputs folds the StartRequest into the environment manager. Callers
750803
// must only invoke it when the inputs actually changed: the manager appends
751804
// overrides and endpoints without ever removing any, so re-folding an unchanged

‎start_request_test.go‎

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -692,3 +692,71 @@ func TestTailWriterKeepsRecentLines(t *testing.T) {
692692
require.NoError(t, err)
693693
require.Equal(t, "dying words", unterminated.Tail())
694694
}
695+
696+
// TestStartRelaunchesWhenInitRepublishesThePort pins that the restart decision
697+
// covers the ports Init owns, not just the StartRequest. An Init between two
698+
// identical Starts can move the service's port; the process bound to the old
699+
// one is not what the caller is being told is running.
700+
func TestStartRelaunchesWhenInitRepublishesThePort(t *testing.T) {
701+
ctx := context.Background()
702+
runtime, env := startTestRuntime(t)
703+
704+
runtime.port = 9000
705+
resp, err := runtime.Start(ctx, &runtimev0.StartRequest{})
706+
require.NoError(t, err)
707+
require.Equal(t, runtimev0.StartStatus_STARTED, resp.GetStatus().GetState(), resp.GetStatus().GetMessage())
708+
require.Len(t, env.started(), 1)
709+
require.Contains(t, env.started()[0].args, "9000")
710+
711+
// What a re-Init resolving a different network mapping publishes.
712+
runtime.port = 9100
713+
resp, err = runtime.Start(ctx, &runtimev0.StartRequest{})
714+
require.NoError(t, err)
715+
require.Equal(t, runtimev0.StartStatus_STARTED, resp.GetStatus().GetState(), resp.GetStatus().GetMessage())
716+
717+
procs := env.started()
718+
require.Len(t, procs, 2, "the process on the superseded port must be replaced")
719+
require.True(t, procs[0].isStopped(), "the process on the old port must be stopped")
720+
require.Contains(t, procs[1].args, "9100")
721+
}
722+
723+
// TestStartDoesNotRefoldAnUnchangedRequest pins the other half: the port change
724+
// above must not be mistaken for a request change. The environment manager
725+
// appends overrides and never removes them, so re-folding an identical request
726+
// would duplicate every entry it carries.
727+
func TestStartDoesNotRefoldAnUnchangedRequest(t *testing.T) {
728+
ctx := context.Background()
729+
runtime, env := startTestRuntime(t)
730+
731+
req := &runtimev0.StartRequest{Overrides: map[string]string{"LOG_LEVEL": "debug"}}
732+
runtime.port = 9000
733+
_, err := runtime.Start(ctx, req)
734+
require.NoError(t, err)
735+
736+
runtime.port = 9100
737+
_, err = runtime.Start(ctx, req)
738+
require.NoError(t, err)
739+
740+
procs := env.started()
741+
require.Len(t, procs, 2)
742+
require.Equal(t, []string{"debug"}, processEnv(procs[1], "LOG_LEVEL"),
743+
"an unchanged request must be folded into the environment manager exactly once")
744+
require.Equal(t, 1, runtime.appliedOverrides["LOG_LEVEL"])
745+
}
746+
747+
// TestStartRefusesAnAbandonedRequest pins that Start does not launch a process
748+
// for a caller that has already given up. Start can wait on initMu for as long
749+
// as an Init takes — a docker pull, a `uv sync` — and the process it launches
750+
// runs on a context of its own, so it would outlive the request entirely.
751+
func TestStartRefusesAnAbandonedRequest(t *testing.T) {
752+
runtime, env := startTestRuntime(t)
753+
754+
ctx, cancel := context.WithCancel(context.Background())
755+
cancel()
756+
757+
resp, err := runtime.Start(ctx, &runtimev0.StartRequest{})
758+
require.NoError(t, err)
759+
require.Equal(t, runtimev0.StartStatus_ERROR, resp.GetStatus().GetState())
760+
require.Empty(t, env.started(), "no process may be launched for an abandoned request")
761+
require.Nil(t, currentRunner(runtime))
762+
}

0 commit comments

Comments
 (0)