Repository: akuity/kargo
Pull request #3739 (2025-03-28 12:04:27)

chore(backport release-1.3): fix: soak time bug
Manual backport of #3731 to `release-1.3` due to merge conflicts.

netlify[bot]: ### <span aria-hidden="true">👷</span> Deploy Preview for *docs-kargo-io* processing.


|  Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">🔨</span> Latest commit | 1ebe2df89cadc0860d8d3e1ff265121f34ec03ae |
|<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/sites/docs-kargo-io/deploys/67e6904eb47bc80008e29693 |
codecov[bot]: ## [Codecov](https://app.codecov.io/gh/akuity/kargo/pull/3739?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=akuity) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Project coverage is 52.74%. Comparing base [(`9b41a28`)](https://app.codecov.io/gh/akuity/kargo/commit/9b41a288ad4bbd2a50d7f1184d6f4dab4ba6952a?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=akuity) to head [(`1ebe2df`)](https://app.codecov.io/gh/akuity/kargo/commit/1ebe2df89cadc0860d8d3e1ff265121f34ec03ae?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=akuity).

<details><summary>Additional details and impacted files</summary>


```diff
@@               Coverage Diff               @@
##           release-1.3    #3739      +/-   ##
===============================================
- Coverage        52.74%   52.74%   -0.01%     
===============================================
  Files              296      296              
  Lines            26856    26855       -1     
===============================================
- Hits             14166    14165       -1     
  Misses           11917    11917              
  Partials           773      773              
```

</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/akuity/kargo/pull/3739?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=akuity).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=akuity).

<details><summary> :rocket: New features to boost your workflow: </summary>

- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
krancour: I noticed this one needed a manual backport. Thank you for beating me to it!

Patch:
diff --git a/api/v1alpha1/stage_helpers.go b/api/v1alpha1/stage_helpers.go
index 2c44a097..19dbc53f 100644
--- a/api/v1alpha1/stage_helpers.go
+++ b/api/v1alpha1/stage_helpers.go
@@ -141,7 +141,7 @@ func (s *Stage) ListAvailableFreight(
 				AvailabilityStrategy: req.Sources.AvailabilityStrategy,
 			}
 			if requiredSoak := req.Sources.RequiredSoakTime; requiredSoak != nil {
-				listOpts.VerifiedBefore = &metav1.Time{Time: time.Now().Add(-requiredSoak.Duration)}
+				listOpts.RequiredSoakTime = &requiredSoak.Duration
 			}
 		}
 		freightFromWarehouse, err := warehouse.ListFreight(ctx, c, listOpts)
diff --git a/api/v1alpha1/warehouse_helpers.go b/api/v1alpha1/warehouse_helpers.go
index e4d24cb3..185c3467 100644
--- a/api/v1alpha1/warehouse_helpers.go
+++ b/api/v1alpha1/warehouse_helpers.go
@@ -82,11 +82,10 @@ type ListWarehouseFreightOptions struct {
 	//
 	// IMPORTANT: This is OR'ed with the ApprovedFor field.
 	VerifiedIn []string
-	// VerifiedBefore optionally specifies a time before which a Freight verified
-	// in any of the Stages named in the VerifiedIn field must have been verified.
-	// This is useful for filtering out Freight whose soak time has not yet
-	// elapsed.
-	VerifiedBefore *metav1.Time
+	// RequiredSoakTime optionally specifies a minimum duration that a piece of
+	// Freight must have continuously remained in a Stage at any time after being
+	// verified.
+	RequiredSoakTime *time.Duration
 	// AvailabilityStrategy specifies the semantics for how Freight is determined
 	// to be available. If not set, the default is to consider Freight available
 	// if it has been verified in any of the provided VerifiedIn stages.
@@ -193,7 +192,7 @@ func (w *Warehouse) ListFreight(
 		return lhs.Name == rhs.Name
 	})
 
-	if len(opts.VerifiedIn) == 0 || opts.VerifiedBefore == nil {
+	if len(opts.VerifiedIn) == 0 || opts.RequiredSoakTime == nil {
 		// Nothing left to do
 		return freight, nil
 	}
@@ -211,11 +210,9 @@ func (w *Warehouse) ListFreight(
 		// Track set of Stages that have passed the verification soak time
 		// for the Freight.
 		verifiedStages := sets.New[string]()
-		for stage, ver := range f.Status.VerifiedIn {
-			if verifiedAt := ver.VerifiedAt; verifiedAt != nil {
-				if verifiedAt.Time.Before(opts.VerifiedBefore.Time) {
-					verifiedStages.Insert(stage)
-				}
+		for stage := range f.Status.VerifiedIn {
+			if f.GetLongestSoak(stage) >= *opts.RequiredSoakTime {
+				verifiedStages.Insert(stage)
 			}
 		}
 


Test patch:
diff --git a/api/v1alpha1/stage_helpers_test.go b/api/v1alpha1/stage_helpers_test.go
index 6969a544..74eacedd 100644
--- a/api/v1alpha1/stage_helpers_test.go
+++ b/api/v1alpha1/stage_helpers_test.go
@@ -335,7 +335,7 @@ func TestStage_ListAvailableFreight(t *testing.T) {
 					Status: FreightStatus{
 						VerifiedIn: map[string]VerifiedStage{
 							testStage: {
-								VerifiedAt: &metav1.Time{Time: time.Now()},
+								LongestCompletedSoak: &metav1.Duration{Duration: 30 * time.Minute},
 							},
 						},
 					},
@@ -349,7 +349,7 @@ func TestStage_ListAvailableFreight(t *testing.T) {
 					Status: FreightStatus{
 						VerifiedIn: map[string]VerifiedStage{
 							testStage: {
-								VerifiedAt: &metav1.Time{Time: time.Now().Add(-time.Hour * 2)},
+								LongestCompletedSoak: &metav1.Duration{Duration: 2 * time.Hour},
 							},
 						},
 					},
diff --git a/api/v1alpha1/warehouse_helpers_test.go b/api/v1alpha1/warehouse_helpers_test.go
index 03791c5f..86dce29d 100644
--- a/api/v1alpha1/warehouse_helpers_test.go
+++ b/api/v1alpha1/warehouse_helpers_test.go
@@ -172,12 +172,11 @@ func TestWarehouse_ListFreight(t *testing.T) {
 			},
 		},
 		{
-			name: "success with VerifiedIn and VerifiedBefore options",
+			name: "success with VerifiedIn and RequiredSoakTime options",
 			opts: &ListWarehouseFreightOptions{
-				ApprovedFor:    testStage,
-				VerifiedIn:     []string{testUpstreamStage},
-				VerifiedBefore: &metav1.Time{Time: time.Now().Add(-1 * time.Hour)},
-			},
+				ApprovedFor:      testStage,
+				VerifiedIn:       []string{testUpstreamStage},
+				RequiredSoakTime: ptr.To(time.Hour)},
 			objects: []client.Object{
 				&Freight{ // This should not be returned
 					ObjectMeta: metav1.ObjectMeta{
@@ -194,7 +193,9 @@ func TestWarehouse_ListFreight(t *testing.T) {
 						ApprovedFor: map[string]ApprovedStage{testStage: {}},
 						// Doesn't matter that it's verified upstream, because this is the
 						// wrong warehouse
-						VerifiedIn: map[string]VerifiedStage{testUpstreamStage: {}},
+						VerifiedIn: map[string]VerifiedStage{testUpstreamStage: {
+							LongestCompletedSoak: &metav1.Duration{Duration: 2 * time.Hour},
+						}},
 					},
 				},
 				&Freight{ // This should not be returned
@@ -236,7 +237,7 @@ func TestWarehouse_ListFreight(t *testing.T) {
 						// yet elapsed
 						VerifiedIn: map[string]VerifiedStage{
 							testUpstreamStage: {
-								VerifiedAt: ptr.To(metav1.Now()),
+								LongestCompletedSoak: &metav1.Duration{Duration: 30 * time.Minute},
 							},
 						},
 					},
@@ -255,7 +256,7 @@ func TestWarehouse_ListFreight(t *testing.T) {
 						// elapsed
 						VerifiedIn: map[string]VerifiedStage{
 							testUpstreamStage: {
-								VerifiedAt: ptr.To(metav1.NewTime(time.Now().Add(-2 * time.Hour))),
+								LongestCompletedSoak: &metav1.Duration{Duration: 2 * time.Hour},
 							},
 						},
 					},
@@ -276,7 +277,7 @@ func TestWarehouse_ListFreight(t *testing.T) {
 				AvailabilityStrategy: FreightAvailabilityStrategyAll,
 				ApprovedFor:          testStage,
 				VerifiedIn:           []string{testUpstreamStage, testUpstreamStage2},
-				VerifiedBefore:       &metav1.Time{Time: time.Now().Add(-1 * time.Hour)},
+				RequiredSoakTime:     ptr.To(time.Hour),
 			},
 			objects: []client.Object{
 				&Freight{ // This should be returned as its approved for the Stage
@@ -292,11 +293,7 @@ func TestWarehouse_ListFreight(t *testing.T) {
 						// This is approved for the Stage
 						ApprovedFor: map[string]ApprovedStage{testStage: {}},
 						// This is only verified in a single upstream Stage
-						VerifiedIn: map[string]VerifiedStage{
-							testUpstreamStage: {
-								VerifiedAt: ptr.To(metav1.Now()),
-							},
-						},
+						VerifiedIn: map[string]VerifiedStage{},
 					},
 				},
 				&Freight{ // This should be returned because its verified in both upstream Stages and soak time has lapsed
@@ -314,10 +311,10 @@ func TestWarehouse_ListFreight(t *testing.T) {
 						// This is verified in all of the upstream Stages and the soak time has lapsed
 						VerifiedIn: map[string]VerifiedStage{
 							testUpstreamStage: {
-								VerifiedAt: ptr.To(metav1.NewTime(time.Now().Add(-2 * time.Hour))),
+								LongestCompletedSoak: &metav1.Duration{Duration: 2 * time.Hour},
 							},
 							testUpstreamStage2: {
-								VerifiedAt: ptr.To(metav1.NewTime(time.Now().Add(-2 * time.Hour))),
+								LongestCompletedSoak: &metav1.Duration{Duration: 2 * time.Hour},
 							},
 						},
 					},
@@ -336,9 +333,7 @@ func TestWarehouse_ListFreight(t *testing.T) {
 						ApprovedFor: map[string]ApprovedStage{},
 						// This is not verified in all of the upstream Stages
 						VerifiedIn: map[string]VerifiedStage{
-							testUpstreamStage: {
-								VerifiedAt: ptr.To(metav1.Now()),
-							},
+							testUpstreamStage: {},
 						},
 					},
 				},
@@ -357,10 +352,10 @@ func TestWarehouse_ListFreight(t *testing.T) {
 						// This is verified in all of the upstream Stages but only passed the soak time of one
 						VerifiedIn: map[string]VerifiedStage{
 							testUpstreamStage: {
-								VerifiedAt: ptr.To(metav1.NewTime(time.Now().Add(-2 * time.Hour))),
+								LongestCompletedSoak: &metav1.Duration{Duration: 2 * time.Hour},
 							},
 							testUpstreamStage2: {
-								VerifiedAt: ptr.To(metav1.Now()),
+								LongestCompletedSoak: &metav1.Duration{Duration: 30 * time.Minute},
 							},
 						},
 					},
@@ -381,7 +376,7 @@ func TestWarehouse_ListFreight(t *testing.T) {
 				AvailabilityStrategy: "Invalid",
 				ApprovedFor:          testStage,
 				VerifiedIn:           []string{testUpstreamStage, testUpstreamStage2},
-				VerifiedBefore:       &metav1.Time{Time: time.Now().Add(-1 * time.Hour)},
+				RequiredSoakTime:     ptr.To(time.Hour),
 			},
 			assertions: func(t *testing.T, _ []Freight, err error) {
 				require.ErrorContains(t, err, "unsupported AvailabilityStrategy")
diff --git a/internal/controller/stages/regular_stages_test.go b/internal/controller/stages/regular_stages_test.go
index 6bac1825..14a5b8ef 100644
--- a/internal/controller/stages/regular_stages_test.go
+++ b/internal/controller/stages/regular_stages_test.go
@@ -4809,7 +4809,7 @@ func TestRegularStageReconciler_autoPromoteFreight(t *testing.T) {
 			},
 		},
 		{
-			name: "handles verified freight from upstream stages with verification duration requirement",
+			name: "handles verified freight from upstream stages with soak time requirement",
 			stage: &kargoapi.Stage{
 				ObjectMeta: metav1.ObjectMeta{
 					Namespace: "fake-project",
@@ -4889,9 +4889,8 @@ func TestRegularStageReconciler_autoPromoteFreight(t *testing.T) {
 					Status: kargoapi.FreightStatus{
 						VerifiedIn: map[string]kargoapi.VerifiedStage{
 							"upstream-stage": {
-								// Should be selected because it was verified
-								// after the required duration.
-								VerifiedAt: &metav1.Time{Time: now.Add(-2 * time.Hour)},
+								// Should be selected because the soak time has elapsed
+								LongestCompletedSoak: &metav1.Duration{Duration: 2 * time.Hour},
 							},
 						},
 					},