engine: fix a race condition in Tiltfile reload

PendingConfigFileChanges had its own change tracking, which didn't get the fixes we applied in #4397 This removes that state completely and unifies Tiltfile change tracking with other stuff
tilt-dev · May 18, 2021 · 7b2ea03 · 7b2ea03
1 parent 388ce5e
commit 7b2ea03
Show file tree

Hide file tree

Showing 7 changed files with 26 additions and 25 deletions.
diff --git a/internal/engine/buildcontrol/build_control.go b/internal/engine/buildcontrol/build_control.go
@@ -20,7 +20,8 @@ func NextTargetToBuild(state store.EngineState) (*store.ManifestTarget, HoldSet)
 
 	// Don't build anything if there are pending config file changes.
 	// We want the Tiltfile to re-run first.
-	if len(state.PendingConfigFileChanges) > 0 {
+	tiltfileHasPendingChanges, _ := state.TiltfileState.HasPendingChanges()
+	if tiltfileHasPendingChanges {
 		holds.Fill(targets, store.HoldTiltfileReload)
 		return nil, holds
 	}

diff --git a/internal/engine/configs/configs_controller.go b/internal/engine/configs/configs_controller.go
@@ -83,10 +83,9 @@ func (cc *ConfigsController) needsBuild(ctx context.Context, st store.RStore) (b
 		reason = reason.With(model.BuildReasonFlagInit)
 	}
 
-	for _, changeTime := range state.PendingConfigFileChanges {
-		if changeTime.After(lastStartTime) {
-			reason = reason.With(model.BuildReasonFlagChangedFiles)
-		}
+	hasPendingChanges, _ := tfState.HasPendingChanges()
+	if hasPendingChanges {
+		reason = reason.With(model.BuildReasonFlagChangedFiles)
 	}
 
 	if state.UserConfigState.ArgsChangeTime.After(lastStartTime) {
@@ -101,9 +100,11 @@ func (cc *ConfigsController) needsBuild(ctx context.Context, st store.RStore) (b
 		return buildEntry{}, false
 	}
 
-	filesChanged := make([]string, 0, len(state.PendingConfigFileChanges))
-	for k := range state.PendingConfigFileChanges {
-		filesChanged = append(filesChanged, k)
+	filesChanged := []string{}
+	for _, st := range state.TiltfileState.BuildStatuses {
+		for k := range st.PendingFileChanges {
+			filesChanged = append(filesChanged, k)
+		}
 	}
 	filesChanged = sliceutils.DedupedAndSorted(filesChanged)
 

diff --git a/internal/engine/configs/configs_controller_test.go b/internal/engine/configs/configs_controller_test.go
@@ -176,7 +176,10 @@ func newCCFixture(t *testing.T) *ccFixture {
 
 	state := st.LockMutableStateForTesting()
 	state.TiltfilePath = f.JoinPath("Tiltfile")
-	state.PendingConfigFileChanges["Tiltfile"] = time.Now()
+	state.TiltfileState.AddPendingFileChange(model.TargetID{
+		Type: model.TargetTypeConfigs,
+		Name: "singleton",
+	}, f.JoinPath("Tiltfile"), time.Now())
 	st.UnlockMutableState()
 
 	return &ccFixture{

diff --git a/internal/engine/fswatch/action.go b/internal/engine/fswatch/action.go
@@ -122,13 +122,6 @@ func processFileWatchStatus(ctx context.Context, state *store.EngineState, fw *f
 		return
 	}
 
-	if targetID.Type == model.TargetTypeConfigs {
-		for _, f := range latestEvent.SeenFiles {
-			state.PendingConfigFileChanges[f] = latestEvent.Time.Time
-		}
-		return
-	}
-
 	mns := state.ManifestNamesForTargetID(targetID)
 	for _, mn := range mns {
 		ms, ok := state.ManifestState(mn)

diff --git a/internal/engine/session/conv.go b/internal/engine/session/conv.go
@@ -265,7 +265,7 @@ func tiltfileTarget(state store.EngineState) session.Target {
 		target.State.Active = &session.TargetStateActive{
 			StartTime: apis.NewMicroTime(state.TiltfileState.CurrentBuild.StartTime),
 		}
-	} else if len(state.PendingConfigFileChanges) != 0 {
+	} else if hasPendingChanges, _ := state.TiltfileState.HasPendingChanges(); hasPendingChanges {
 		target.State.Waiting = &session.TargetStateWaiting{
 			WaitReason: "config-changed",
 		}

diff --git a/internal/engine/upper.go b/internal/engine/upper.go
@@ -672,10 +672,9 @@ func handleConfigsReloaded(
 	state.UpdateSettings = event.UpdateSettings
 
 	// Remove pending file changes that were consumed by this build.
-	for file, modTime := range state.PendingConfigFileChanges {
-		if timecmp.BeforeOrEqual(modTime, state.TiltfileState.LastBuild().StartTime) {
-			delete(state.PendingConfigFileChanges, file)
-		}
+	startTime := state.TiltfileState.LastBuild().StartTime
+	for _, status := range state.TiltfileState.BuildStatuses {
+		status.ClearPendingChangesBefore(startTime)
 	}
 }
 

diff --git a/internal/store/engine_state.go b/internal/store/engine_state.go
@@ -87,8 +87,6 @@ type EngineState struct {
 	Tiltignore    model.Dockerignore
 	WatchSettings model.WatchSettings
 
-	PendingConfigFileChanges map[string]time.Time
-
 	TriggerQueue []model.ManifestName
 
 	IsProfiling bool
@@ -154,6 +152,10 @@ func (e *EngineState) AnalyticsEffectiveOpt() analytics.Opt {
 }
 
 func (e *EngineState) ManifestNamesForTargetID(id model.TargetID) []model.ManifestName {
+	if id.Type == model.TargetTypeConfigs {
+		return []model.ManifestName{model.TiltfileManifestName}
+	}
+
 	result := make([]model.ManifestName, 0)
 	for mn, mt := range e.ManifestTargets {
 		manifest := mt.Manifest
@@ -475,15 +477,17 @@ func NewState() *EngineState {
 	ret := &EngineState{}
 	ret.LogStore = logstore.NewLogStore()
 	ret.ManifestTargets = make(map[model.ManifestName]*ManifestTarget)
-	ret.PendingConfigFileChanges = make(map[string]time.Time)
 	ret.Secrets = model.SecretSet{}
 	ret.DockerPruneSettings = model.DefaultDockerPruneSettings()
 	ret.VersionSettings = model.VersionSettings{
 		CheckUpdates: true,
 	}
 	ret.UpdateSettings = model.DefaultUpdateSettings()
 	ret.CurrentlyBuilding = make(map[model.ManifestName]bool)
-	ret.TiltfileState = &ManifestState{}
+	ret.TiltfileState = &ManifestState{
+		Name:          model.TiltfileManifestName,
+		BuildStatuses: make(map[model.TargetID]*BuildStatus),
+	}
 
 	if ok, _ := tiltanalytics.IsAnalyticsDisabledFromEnv(); ok {
 		ret.AnalyticsEnvOpt = analytics.OptOut