Skip to content

Commit e0b29d8

Browse files
leighmccullochleighmcculloch
committed
refactor planBuild logic
1 parent 74b6b20 commit e0b29d8

2 files changed

Lines changed: 142 additions & 36 deletions

File tree

‎run/run.go‎

Lines changed: 90 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,44 @@ type Options struct {
4949
Stderr io.Writer
5050
}
5151

52+
// buildAction is how Run obtains the image to run when the desired image may
53+
// not already exist.
54+
type buildAction int
55+
56+
const (
57+
actionUseDesired buildAction = iota // desired image is cached; run it
58+
actionSyncBuild // build the desired image synchronously, then run it
59+
actionUseFallback // run an older fallback image now; build desired in background
60+
)
61+
62+
// buildPlan is the outcome of planBuild: what to run, and which image (if any)
63+
// to build in the background.
64+
type buildPlan struct {
65+
action buildAction
66+
bgBuildTag string // image tag to build in the background ("" = none)
67+
}
68+
69+
// planBuild decides how Run should obtain a runnable image given cache state.
70+
//
71+
// The fallback case is the subtle one: when the desired image is absent but a
72+
// structurally-identical older image exists (only the tool version differs),
73+
// Run uses the old image immediately and must launch a background build for the
74+
// desired image. Otherwise — since the cached version already equals the latest
75+
// fetched version — nothing else would ever build it and Run would fall back to
76+
// the old image forever.
77+
func planBuild(imageTag string, imageExists, explicitRebuild, fallbackUsable bool) buildPlan {
78+
switch {
79+
case imageExists:
80+
return buildPlan{action: actionUseDesired}
81+
case explicitRebuild:
82+
return buildPlan{action: actionSyncBuild}
83+
case fallbackUsable:
84+
return buildPlan{action: actionUseFallback, bgBuildTag: imageTag}
85+
default:
86+
return buildPlan{action: actionSyncBuild}
87+
}
88+
}
89+
5290
// Tool runs a tool inside a container.
5391
func Tool(opts Options) error {
5492
tool := opts.ToolDef.Name
@@ -316,7 +354,41 @@ func Tool(opts Options) error {
316354
})
317355
}
318356

319-
if imageExists {
357+
// launchBgBuild spawns a detached build for the given image tag unless one
358+
// is already in flight.
359+
launchBgBuild := func(tag string, bArgs map[string]string) {
360+
if IsBuilding(tag) {
361+
return
362+
}
363+
_ = LaunchBackgroundBuild(BackgroundBuildOptions{
364+
ImageTag: tag,
365+
Dockerfile: dockerfile,
366+
BuildArgs: bArgs,
367+
Backend: cfg.Backend,
368+
FlyApp: cfg.Backends.Fly.App,
369+
FlyRegion: cfg.Backends.Fly.Region,
370+
NamespaceSize: cfg.Backends.Namespace.Size,
371+
NamespaceSite: cfg.Backends.Namespace.Site,
372+
Tool: tool,
373+
})
374+
}
375+
376+
// Determine whether a structurally-identical older image is usable as a
377+
// fallback while the desired image builds. Only relevant when the desired
378+
// image is absent and we're not doing an explicit rebuild, so we avoid the
379+
// extra ImageExists call otherwise.
380+
explicitRebuild := opts.ForceBuild || opts.NoCache || opts.ToolVersion != ""
381+
fallbackTag := loadLastImage(tool)
382+
fallbackUsable := false
383+
if !imageExists && !explicitRebuild && fallbackTag != "" && fallbackTag != imageTag {
384+
if exists, _ := backendClient.ImageExists(ctx, fallbackTag); exists {
385+
fallbackUsable = structuralTag == loadStructuralTag(tool)
386+
}
387+
}
388+
389+
plan := planBuild(imageTag, imageExists, explicitRebuild, fallbackUsable)
390+
switch plan.action {
391+
case actionUseDesired:
320392
// Image is cached — use it directly (scenario 2 & 3 cache hit).
321393
if err := buildEnvironment(ctx, backendClient, buildEnvOptions{
322394
tool: tool,
@@ -340,36 +412,28 @@ func Tool(opts Options) error {
340412
}
341413
return err
342414
}
343-
} else if opts.ForceBuild || opts.NoCache || opts.ToolVersion != "" {
344-
// Scenario 3 cache miss, or explicit rebuild — build synchronously.
415+
case actionUseFallback:
416+
// Desired image is missing but a structurally-identical older image
417+
// exists (only the tool version differs). Run it now; the desired
418+
// image builds in the background (launched below via plan.bgBuildTag).
419+
logSection("Using cached environment (update building in background)...")
420+
runImageTag = fallbackTag
421+
case actionSyncBuild:
422+
// Explicit rebuild, or no usable fallback — build synchronously.
345423
if err := syncBuild(); err != nil {
346424
if progress != nil {
347425
progress.Complete()
348426
}
349427
return err
350428
}
351-
} else {
352-
// Image doesn't exist — the background build from a previous run
353-
// may not have finished yet. Use the previous image as a fallback
354-
// if only the version changed; otherwise build synchronously.
355-
fallbackTag := loadLastImage(tool)
356-
var fallbackExists bool
357-
if fallbackTag != "" && fallbackTag != imageTag {
358-
fallbackExists, _ = backendClient.ImageExists(ctx, fallbackTag)
359-
}
360-
361-
onlyVersionChanged := fallbackExists && structuralTag == loadStructuralTag(tool)
362-
if onlyVersionChanged {
363-
logSection("Using cached environment (update building in background)...")
364-
runImageTag = fallbackTag
365-
} else {
366-
if err := syncBuild(); err != nil {
367-
if progress != nil {
368-
progress.Complete()
369-
}
370-
return err
371-
}
372-
}
429+
}
430+
431+
// The desired image was missing and we fell back to an older one — relaunch
432+
// its build here. The version-change goroutine below won't, because the
433+
// cached version already equals the latest fetched version, so without this
434+
// we'd fall back forever.
435+
if plan.bgBuildTag != "" {
436+
launchBgBuild(plan.bgBuildTag, buildArgs)
373437
}
374438

375439
// Save state after image is resolved but before the tool runs, so
@@ -397,17 +461,7 @@ func Tool(opts Options) error {
397461
}
398462
newBuildArgs["TOOL_VERSION"] = newVersion
399463
logSection("New version available (%s), building in background...", newVersion)
400-
_ = LaunchBackgroundBuild(BackgroundBuildOptions{
401-
ImageTag: newImageTag,
402-
Dockerfile: dockerfile,
403-
BuildArgs: newBuildArgs,
404-
Backend: cfg.Backend,
405-
FlyApp: cfg.Backends.Fly.App,
406-
FlyRegion: cfg.Backends.Fly.Region,
407-
NamespaceSize: cfg.Backends.Namespace.Size,
408-
NamespaceSite: cfg.Backends.Namespace.Site,
409-
Tool: tool,
410-
})
464+
launchBgBuild(newImageTag, newBuildArgs)
411465
}()
412466
}
413467

‎run/run_test.go‎

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -66,6 +66,58 @@ func TestRepoURLMatches(t *testing.T) {
6666
}
6767
}
6868

69+
func TestPlanBuild(t *testing.T) {
70+
const desired = "silo-claude-abc123-2.1.150"
71+
72+
tests := []struct {
73+
name string
74+
imageExists bool
75+
explicitRebuild bool
76+
fallbackUsable bool
77+
wantAction buildAction
78+
wantBgBuildTag string
79+
}{
80+
{
81+
name: "desired image cached runs it directly",
82+
imageExists: true,
83+
wantAction: actionUseDesired,
84+
},
85+
{
86+
name: "explicit rebuild builds synchronously",
87+
explicitRebuild: true,
88+
wantAction: actionSyncBuild,
89+
},
90+
{
91+
name: "no usable fallback builds synchronously",
92+
wantAction: actionSyncBuild,
93+
wantBgBuildTag: "",
94+
},
95+
{
96+
// Regression: the cached version's image went missing (a prior
97+
// background build never completed), but a structurally-identical
98+
// older image exists. We must run the fallback AND relaunch the
99+
// background build for the desired image — otherwise we fall back
100+
// forever while reporting the new version as cached.
101+
name: "missing desired image with usable fallback relaunches background build",
102+
fallbackUsable: true,
103+
wantAction: actionUseFallback,
104+
wantBgBuildTag: desired,
105+
},
106+
}
107+
108+
for _, tt := range tests {
109+
t.Run(tt.name, func(t *testing.T) {
110+
got := planBuild(desired, tt.imageExists, tt.explicitRebuild, tt.fallbackUsable)
111+
if got.action != tt.wantAction {
112+
t.Errorf("action = %v, want %v", got.action, tt.wantAction)
113+
}
114+
if got.bgBuildTag != tt.wantBgBuildTag {
115+
t.Errorf("bgBuildTag = %q, want %q", got.bgBuildTag, tt.wantBgBuildTag)
116+
}
117+
})
118+
}
119+
}
120+
69121
func TestCollectMountsIncludesAdditionalCLIMounts(t *testing.T) {
70122
home := t.TempDir()
71123
oldHome := os.Getenv("HOME")

0 commit comments

Comments
 (0)