fix: freight soak time verification with unrelated and control-flow stages

Auto-promotion could either incorrectly pass or permanently stall the
soak time gate (Stage.spec.requestedFreight[].sources.requiredSoakTime),
depending on the upstream Stage topology. Two distinct problems are
addressed:

1. Soak accrued in unrelated Stages satisfied the gate.

   ListFreightFromWarehouse iterated over every Stage in the Freight's
   status.verifiedIn map when checking whether the required soak time
   had elapsed. Soak time accumulated in Stages that were NOT named as
   upstream sources could therefore satisfy the requirement, letting
   Freight promote before it had actually soaked in the configured
   upstream Stage(s). Restrict the check to the Stages named in the
   source's `stages` list (opts.VerifiedIn).

2. A control-flow upstream Stage could never satisfy the gate.

   Control-flow Stages verify Freight without ever holding it: the
   Freight is never added to status.currentlyIn and no completed soak is
   ever recorded. GetLongestSoak only consulted those two fields, so it
   returned 0 for a control-flow Stage no matter how long ago the Freight
   was verified there. With problem 1 fixed, a downstream Stage whose
   upstream source is a control-flow Stage could then never pass its
   requiredSoakTime, and auto-promotion stalled indefinitely.

   When neither a current residency nor a completed soak is recorded for
   a Stage in which the Freight is verified -- which uniquely identifies
   a control-flow Stage -- measure the soak from the Freight's verifiedAt
   timestamp instead. Regular Stages always have one of the two records,
   so their behavior is unchanged.

Refs: #4586
Signed-off-by: Justin Chen <mail@justin0u0.com>

Author: justin0u0

api/v1alpha1/freight_types.go

@@ -153,16 +153,26 @@ func (f *Freight) IsApprovedFor(stage string) bool {
 // the current soak time is calculated and compared to the longest completed
 // soak time on record.
 func (f *Freight) GetLongestSoak(stage string) time.Duration {
-	if _, verified := f.Status.VerifiedIn[stage]; !verified {
+	record, verified := f.Status.VerifiedIn[stage]
+	if !verified {
 		return 0
 	}
 	var longestCompleted time.Duration
-	if record, isVerified := f.Status.VerifiedIn[stage]; isVerified && record.LongestCompletedSoak != nil {
+	if record.LongestCompletedSoak != nil {
 		longestCompleted = record.LongestCompletedSoak.Duration
 	}
 	var current time.Duration
-	if record, isCurrent := f.Status.CurrentlyIn[stage]; isCurrent {
-		current = time.Since(record.Since.Time)
+	if c, isCurrent := f.Status.CurrentlyIn[stage]; isCurrent && c.Since != nil {
+		current = time.Since(c.Since.Time)
+	}
+	// Control-flow Stages verify Freight without ever holding it: the Freight is
+	// never added to CurrentlyIn and no soak is ever completed. For such Stages,
+	// neither a current nor a completed soak is recorded, so measure the soak
+	// from the time the Freight was verified. A regular Stage always has either
+	// a current residency or a completed soak, so this branch does not affect
+	// it.
+	if longestCompleted == 0 && current == 0 && record.VerifiedAt != nil {
+		return time.Since(record.VerifiedAt.Time)
 	}
 	return time.Duration(max(longestCompleted.Nanoseconds(), current.Nanoseconds()))
 }

api/v1alpha1/freight_types_test.go

@@ -316,6 +316,21 @@ func TestFreight_GetLongestSoak(t *testing.T) {
 				require.LessOrEqual(t, longestSoak, 2*time.Hour+time.Second)
 			},
 		},
+		{
+			// A control-flow Stage verifies Freight without ever holding it, so
+			// neither a current residency nor a completed soak is recorded. Soak
+			// must be measured from the time the Freight was verified.
+			name: "Freight verified in a control-flow Stage; soak measured from verification",
+			status: FreightStatus{
+				VerifiedIn: map[string]VerifiedStage{
+					testStage: {VerifiedAt: &metav1.Time{Time: time.Now().Add(-2 * time.Hour)}},
+				},
+			},
+			assertions: func(t *testing.T, _ FreightStatus, longestSoak time.Duration) {
+				require.GreaterOrEqual(t, longestSoak, 2*time.Hour)
+				require.LessOrEqual(t, longestSoak, 2*time.Hour+time.Second)
+			},
+		},
 	}
 	for _, testCase := range testCases {
 		t.Run(testCase.name, func(t *testing.T) {

pkg/api/warehouse.go

@@ -225,10 +225,15 @@ func ListFreightFromWarehouse(
 			}
 		}
 
-		// Track set of Stages that have passed the verification soak time
-		// for the Freight.
+		// Track the set of configured upstream Stages in which the Freight
+		// has been verified AND has passed the required soak time. Only
+		// upstream Stages named in opts.VerifiedIn are considered -- soak
+		// time accrued in unrelated Stages must not satisfy this gate.
 		verifiedStages := sets.New[string]()
-		for stage := range f.Status.VerifiedIn {
+		for _, stage := range opts.VerifiedIn {
+			if _, verified := f.Status.VerifiedIn[stage]; !verified {
+				continue
+			}
 			if f.HasSoakedIn(stage, opts.RequiredSoakTime) {
 				verifiedStages.Insert(stage)
 			}

pkg/api/warehouse_test.go

@@ -452,6 +452,62 @@ func TestListFreightFromWarehouse(t *testing.T) {
 				require.Equal(t, "fake-freight-2", freight[0].Name)
 			},
 		},
+		{
+			name: "soak in unrelated Stage does not satisfy soak gate",
+			opts: &ListWarehouseFreightOptions{
+				VerifiedIn:       []string{testUpstreamStage},
+				RequiredSoakTime: &metav1.Duration{Duration: time.Hour},
+			},
+			objects: []client.Object{
+				&kargoapi.Freight{
+					// Verified in the configured upstream Stage but has not
+					// soaked there long enough. It HAS soaked > 1h in an
+					// unrelated Stage, which must not count.
+					ObjectMeta: metav1.ObjectMeta{
+						Namespace: testProject,
+						Name:      "fake-freight-1",
+					},
+					Origin: kargoapi.FreightOrigin{
+						Kind: kargoapi.FreightOriginKindWarehouse,
+						Name: testWarehouse,
+					},
+					Status: kargoapi.FreightStatus{
+						VerifiedIn: map[string]kargoapi.VerifiedStage{
+							testUpstreamStage: {
+								LongestCompletedSoak: &metav1.Duration{Duration: 30 * time.Minute},
+							},
+							"unrelated-stage": {
+								LongestCompletedSoak: &metav1.Duration{Duration: 2 * time.Hour},
+							},
+						},
+					},
+				},
+				&kargoapi.Freight{
+					// Verified in the configured upstream Stage with sufficient
+					// soak time -- should be returned.
+					ObjectMeta: metav1.ObjectMeta{
+						Namespace: testProject,
+						Name:      "fake-freight-2",
+					},
+					Origin: kargoapi.FreightOrigin{
+						Kind: kargoapi.FreightOriginKindWarehouse,
+						Name: testWarehouse,
+					},
+					Status: kargoapi.FreightStatus{
+						VerifiedIn: map[string]kargoapi.VerifiedStage{
+							testUpstreamStage: {
+								LongestCompletedSoak: &metav1.Duration{Duration: 2 * time.Hour},
+							},
+						},
+					},
+				},
+			},
+			assertions: func(t *testing.T, freight []kargoapi.Freight, err error) {
+				require.NoError(t, err)
+				require.Len(t, freight, 1)
+				require.Equal(t, "fake-freight-2", freight[0].Name)
+			},
+		},
 		{
 			name: "failure with invalid AvailabilityStrategy",
 			opts: &ListWarehouseFreightOptions{