store: update how we model file changes (#6503)

before this PR, we kept a map of recent file changes,
and deleted from the map when the changes were consumed.

this creates race conditions if the events are processed
in a different order than the changes, e.g.,
if a "consumed" event at 12:02 is processed before
the file change event at 12:01.

after this PR, we record both the file change time
and the consumed time, then compare them to determine
if the file change is still pending.

Signed-off-by: Nick Santos <[email protected]>

Author: nicks

internal/controllers/core/tiltfile/reducers.go

@@ -73,7 +73,7 @@ func HandleConfigsReloaded(
 
 	// Remove pending file changes that were consumed by this build.
 	for _, status := range ms.BuildStatuses {
-		status.ClearPendingChangesBefore(b.StartTime)
+		status.ConsumeChangesBefore(b.StartTime)
 	}
 
 	// Track the new secrets and go back to scrub them.

internal/engine/buildcontrol/build_control.go

@@ -155,15 +155,15 @@ func waitingOnDependencies(state store.EngineState, mt *store.ManifestTarget) []
 // 3) Checks that the image still exists on the image store
 //
 // But in this particular context, we can cheat a bit.
-func canReuseImageTargetHeuristic(spec model.TargetSpec, status store.BuildStatus) bool {
+func canReuseImageTargetHeuristic(spec model.TargetSpec, status *store.BuildStatus) bool {
 	id := spec.ID()
 	if id.Type != model.TargetTypeImage {
 		return false
 	}
 
 	// NOTE(nick): A more accurate check might see if the pending file changes
 	// are potentially live-updatable, but this is OK for the case of a base image.
-	if len(status.PendingFileChanges) > 0 || len(status.PendingDependencyChanges) > 0 {
+	if status.HasPendingFileChanges() || status.HasPendingDependencyChanges() {
 		return false
 	}
 
@@ -505,14 +505,14 @@ func IsLiveUpdateTargetWaitingOnDeploy(state store.EngineState, mt *store.Manife
 
 	// Go through all the files, and make sure they're live-update-able.
 	for id, status := range mt.State.BuildStatuses {
-		if len(status.PendingFileChanges) == 0 {
+		if !status.HasPendingFileChanges() {
 			continue
 		}
 
 		// We have an image target with changes!
 		// First, make sure that all the changes match a sync.
-		files := make([]string, 0, len(status.PendingFileChanges))
-		for f := range status.PendingFileChanges {
+		files := []string{}
+		for f := range status.PendingFileChanges() {
 			files = append(files, f)
 		}
 
@@ -605,7 +605,7 @@ func HoldLiveUpdateTargetsHandledByReconciler(state store.EngineState, mts []*st
 		// Changes to the deploy target can't be live-updated.
 		if mt.Manifest.DeployTarget != nil {
 			bs, hasBuildStatus := mt.State.BuildStatuses[mt.Manifest.DeployTarget.ID()]
-			hasPendingChanges := hasBuildStatus && len(bs.PendingFileChanges) > 0
+			hasPendingChanges := hasBuildStatus && bs.HasPendingFileChanges()
 			if hasPendingChanges {
 				continue
 			}
@@ -615,7 +615,7 @@ func HoldLiveUpdateTargetsHandledByReconciler(state store.EngineState, mts []*st
 		iTargets := mt.Manifest.ImageTargets
 		for _, iTarget := range iTargets {
 			bs, hasBuildStatus := mt.State.BuildStatuses[iTarget.ID()]
-			hasPendingChanges := hasBuildStatus && len(bs.PendingFileChanges) > 0
+			hasPendingChanges := hasBuildStatus && bs.HasPendingFileChanges()
 			if !hasPendingChanges {
 				continue
 			}

internal/engine/buildcontrol/build_control_test.go

@@ -322,7 +322,7 @@ func TestLiveUpdateMainImageHold(t *testing.T) {
 	}
 	f.st.KubernetesResources["sancho"] = resource
 
-	sancho.State.MutableBuildStatus(sanchoImage.ID()).PendingFileChanges[srcFile] = time.Now()
+	sancho.State.MutableBuildStatus(sanchoImage.ID()).FileChanges[srcFile] = time.Now()
 	f.assertNoTargetNextToBuild()
 	f.assertHold("sancho", store.HoldReasonReconciling)
 
@@ -338,7 +338,7 @@ func TestLiveUpdateMainImageHold(t *testing.T) {
 	f.assertNoTargetNextToBuild()
 
 	// If the base image has a change, we have to rebuild.
-	sancho.State.MutableBuildStatus(baseImage.ID()).PendingFileChanges[srcFile] = time.Now()
+	sancho.State.MutableBuildStatus(baseImage.ID()).FileChanges[srcFile] = time.Now()
 	f.assertNextTargetToBuild("sancho")
 }
 
@@ -385,7 +385,7 @@ func TestLiveUpdateBaseImageHold(t *testing.T) {
 	}
 	f.st.KubernetesResources["sancho"] = resource
 
-	sancho.State.MutableBuildStatus(baseImage.ID()).PendingFileChanges[srcFile] = time.Now()
+	sancho.State.MutableBuildStatus(baseImage.ID()).FileChanges[srcFile] = time.Now()
 	f.assertNoTargetNextToBuild()
 	f.assertHold("sancho", store.HoldReasonReconciling)
 
@@ -401,7 +401,7 @@ func TestLiveUpdateBaseImageHold(t *testing.T) {
 	f.assertNoTargetNextToBuild()
 
 	// If the deploy image has a change, we have to rebuild.
-	sancho.State.MutableBuildStatus(sanchoImage.ID()).PendingFileChanges[srcFile] = time.Now()
+	sancho.State.MutableBuildStatus(sanchoImage.ID()).FileChanges[srcFile] = time.Now()
 	f.assertNextTargetToBuild("sancho")
 }
 
@@ -472,15 +472,15 @@ func TestHoldForDeploy(t *testing.T) {
 
 	status := sancho.State.MutableBuildStatus(sanchoImage.ID())
 
-	status.PendingFileChanges[objFile] = time.Now()
+	status.FileChanges[objFile] = time.Now().Add(-2 * time.Second)
 	f.assertNextTargetToBuild("sancho")
-	delete(status.PendingFileChanges, objFile)
+	status.ConsumedChanges = time.Now().Add(-2 * time.Second)
 
-	status.PendingFileChanges[fallbackFile] = time.Now()
+	status.FileChanges[fallbackFile] = time.Now().Add(-time.Second)
 	f.assertNextTargetToBuild("sancho")
-	delete(status.PendingFileChanges, fallbackFile)
+	status.ConsumedChanges = time.Now().Add(-time.Second)
 
-	status.PendingFileChanges[srcFile] = time.Now()
+	status.FileChanges[srcFile] = time.Now()
 	f.assertNoTargetNextToBuild()
 	f.assertHold("sancho", store.HoldReasonWaitingForDeploy)
 
@@ -544,11 +544,11 @@ func TestHoldForManualLiveUpdate(t *testing.T) {
 	// This shouldn't trigger a full-build, because it will be handled by the live-updater.
 	status := sancho.State.MutableBuildStatus(sanchoImage.ID())
 	f.st.AppendToTriggerQueue(sancho.Manifest.Name, model.BuildReasonFlagTriggerCLI)
-	status.PendingFileChanges[srcFile] = time.Now()
+	status.FileChanges[srcFile] = time.Now()
 	f.assertNoTargetNextToBuild()
 
 	// This should trigger a full-rebuild, because we have a trigger without pending changes.
-	delete(status.PendingFileChanges, srcFile)
+	status.ConsumedChanges = time.Now()
 	f.assertNextTargetToBuild("sancho")
 }
 

internal/engine/buildcontroller.go

@@ -218,13 +218,13 @@ func buildStateSet(ctx context.Context, manifest model.Manifest,
 		id := spec.ID()
 		status := ms.BuildStatus(id)
 		var filesChanged []string
-		for file := range status.PendingFileChanges {
+		for file := range status.PendingFileChanges() {
 			filesChanged = append(filesChanged, file)
 		}
 		sort.Strings(filesChanged)
 
 		var depsChanged []model.TargetID
-		for dep := range status.PendingDependencyChanges {
+		for dep := range status.PendingDependencyChanges() {
 			depsChanged = append(depsChanged, dep)
 		}
 

internal/engine/buildcontroller_test.go

@@ -113,7 +113,7 @@ func TestTriggerModes(t *testing.T) {
 
 			f.fsWatcher.Events <- watch.NewFileEvent(f.JoinPath("main.go"))
 			f.WaitUntil("pending change appears", func(st store.EngineState) bool {
-				return len(st.BuildStatus(manifest.ImageTargetAt(0).ID()).PendingFileChanges) >= 1
+				return st.BuildStatus(manifest.ImageTargetAt(0).ID()).CountPendingFileChanges() >= 1
 			})
 
 			if !tc.expectBuildWhenFilesChange {
@@ -161,7 +161,7 @@ func TestBuildControllerImageBuildTrigger(t *testing.T) {
 				f.fsWatcher.Events <- watch.NewFileEvent(f.JoinPath("main.go"))
 			}
 			f.WaitUntil("pending change appears", func(st store.EngineState) bool {
-				return len(st.BuildStatus(manifest.ImageTargetAt(0).ID()).PendingFileChanges) >= len(expectedFiles)
+				return st.BuildStatus(manifest.ImageTargetAt(0).ID()).CountPendingFileChanges() >= len(expectedFiles)
 			})
 
 			if manifest.TriggerMode.AutoOnChange() {
@@ -214,10 +214,10 @@ func TestBuildQueueOrdering(t *testing.T) {
 
 	f.fsWatcher.Events <- watch.NewFileEvent(f.JoinPath("main.go"))
 	f.WaitUntil("pending change appears", func(st store.EngineState) bool {
-		return len(st.BuildStatus(m1.ImageTargetAt(0).ID()).PendingFileChanges) > 0 &&
-			len(st.BuildStatus(m2.ImageTargetAt(0).ID()).PendingFileChanges) > 0 &&
-			len(st.BuildStatus(m3.ImageTargetAt(0).ID()).PendingFileChanges) > 0 &&
-			len(st.BuildStatus(m4.ImageTargetAt(0).ID()).PendingFileChanges) > 0
+		return st.BuildStatus(m1.ImageTargetAt(0).ID()).HasPendingFileChanges() &&
+			st.BuildStatus(m2.ImageTargetAt(0).ID()).HasPendingFileChanges() &&
+			st.BuildStatus(m3.ImageTargetAt(0).ID()).HasPendingFileChanges() &&
+			st.BuildStatus(m4.ImageTargetAt(0).ID()).HasPendingFileChanges()
 	})
 	f.assertNoCall("even tho there are pending changes, manual manifest shouldn't build w/o explicit trigger")
 
@@ -270,10 +270,10 @@ func TestBuildQueueAndAutobuildOrdering(t *testing.T) {
 
 	f.fsWatcher.Events <- watch.NewFileEvent(f.JoinPath("dirManual/main.go"))
 	f.WaitUntil("pending change appears", func(st store.EngineState) bool {
-		return len(st.BuildStatus(m1.ImageTargetAt(0).ID()).PendingFileChanges) > 0 &&
-			len(st.BuildStatus(m2.ImageTargetAt(0).ID()).PendingFileChanges) > 0 &&
-			len(st.BuildStatus(m3.ImageTargetAt(0).ID()).PendingFileChanges) > 0 &&
-			len(st.BuildStatus(m4.ImageTargetAt(0).ID()).PendingFileChanges) > 0
+		return st.BuildStatus(m1.ImageTargetAt(0).ID()).HasPendingFileChanges() &&
+			st.BuildStatus(m2.ImageTargetAt(0).ID()).HasPendingFileChanges() &&
+			st.BuildStatus(m3.ImageTargetAt(0).ID()).HasPendingFileChanges() &&
+			st.BuildStatus(m4.ImageTargetAt(0).ID()).HasPendingFileChanges()
 	})
 	f.assertNoCall("even tho there are pending changes, manual manifest shouldn't build w/o explicit trigger")
 

internal/engine/upper_test.go

@@ -1093,7 +1093,7 @@ k8s_yaml('snack.yaml')`
 
 	f.fsWatcher.Events <- watch.NewFileEvent(f.JoinPath("src/main.go"))
 	f.WaitUntil("pending change appears", func(st store.EngineState) bool {
-		return len(st.BuildStatus(imageTargetID).PendingFileChanges) > 0
+		return st.BuildStatus(imageTargetID).HasPendingFileChanges()
 	})
 	f.assertNoCall("even tho there are pending changes, manual manifest shouldn't build w/o explicit trigger")
 

internal/hud/view.go

@@ -51,7 +51,7 @@ func StateToTerminalView(s store.EngineState, mu *sync.RWMutex) view.View {
 
 		var pendingBuildEdits []string
 		for _, status := range ms.BuildStatuses {
-			for f := range status.PendingFileChanges {
+			for f := range status.PendingFileChanges() {
 				pendingBuildEdits = append(pendingBuildEdits, f)
 			}
 		}

internal/hud/view_test.go

@@ -47,7 +47,7 @@ func TestStateToViewRelativeEditPaths(t *testing.T) {
 			},
 		},
 	}
-	ms.MutableBuildStatus(m.ImageTargets[0].ID()).PendingFileChanges =
+	ms.MutableBuildStatus(m.ImageTargets[0].ID()).FileChanges =
 		map[string]time.Time{
 			f.JoinPath("a", "b", "c", "foo"):    time.Now(),
 			f.JoinPath("a", "b", "c", "d", "e"): time.Now(),

internal/hud/webview/convert_test.go

@@ -58,7 +58,7 @@ func TestStateToWebViewRelativeEditPaths(t *testing.T) {
 	ms.BuildHistory = []model.BuildRecord{
 		{},
 	}
-	ms.MutableBuildStatus(m.ImageTargets[0].ID()).PendingFileChanges =
+	ms.MutableBuildStatus(m.ImageTargets[0].ID()).FileChanges =
 		map[string]time.Time{
 			f.JoinPath("a", "b", "c", "foo"):    time.Now(),
 			f.JoinPath("a", "b", "c", "d", "e"): time.Now(),

internal/store/buildcontrols/reducers.go

@@ -81,7 +81,7 @@ func handleBuildResults(engineState *store.EngineState,
 
 	// Remove pending file changes that were consumed by this build.
 	for _, status := range ms.BuildStatuses {
-		status.ClearPendingChangesBefore(br.StartTime)
+		status.ConsumeChangesBefore(br.StartTime)
 	}
 
 	if isBuildSuccess {
@@ -124,7 +124,7 @@ func handleBuildResults(engineState *store.EngineState,
 
 			currentStatus := currentMS.MutableBuildStatus(id)
 			currentStatus.LastResult = result
-			currentStatus.ClearPendingChangesBefore(br.StartTime)
+			currentStatus.ConsumeChangesBefore(br.StartTime)
 			updatedIDSet[id] = true
 		}
 
@@ -155,7 +155,7 @@ func handleBuildResults(engineState *store.EngineState,
 				}
 
 				// Otherwise, we need to mark it for rebuild to pick up the new image.
-				currentMS.MutableBuildStatus(rDepID).PendingDependencyChanges[updatedID] = br.StartTime
+				currentMS.MutableBuildStatus(rDepID).DependencyChanges[updatedID] = br.StartTime
 			}
 		}
 	}