fix: lastTransitionedTime never populated due to self-comparison (#3001)

Author: adityathebe

pkg/api/run_now.go

@@ -4,6 +4,7 @@ import (
 	"errors"
 	"fmt"
 	"net/http"
+	"time"
 
 	"github.com/flanksource/canary-checker/api/context"
 	v1 "github.com/flanksource/canary-checker/api/v1"
@@ -58,6 +59,10 @@ func RunCanaryHandler(c echo.Context) error {
 		return nil
 	}
 
+	// Capture the cutoff before persisting so LatestCheckStatus excludes the
+	// current run's just-saved status and compares against the prior state.
+	now := time.Now()
+
 	for _, result := range results {
 		if _, err := cache.PostgresCache.Add(ctx.Context, pkg.FromV1(result.Canary, result.Check), pkg.CheckStatusFromResult(*result)); err != nil {
 			return errorResponse(c, err, http.StatusInternalServerError)
@@ -68,7 +73,7 @@ func RunCanaryHandler(c echo.Context) error {
 		}
 	}
 
-	canaryJobs.UpdateCanaryStatusAndEvent(ctx.Context, *canary, results)
+	canaryJobs.UpdateCanaryStatusAndEvent(ctx.Context, *canary, results, now)
 	return c.JSON(http.StatusOK, results[0])
 }
 

pkg/db/canary.go

@@ -174,12 +174,20 @@ func GetTransformedCheckIDs(ctx context.Context, canaryID string, excludeTypes .
 	return ids, err
 }
 
-func LatestCheckStatus(ctx context.Context, checkID string) (*models.CheckStatus, error) {
+// LatestCheckStatus returns the most recent check_statuses row for the given
+// check that was recorded strictly before the given cutoff. The cutoff must
+// predate the current run's persisted status so that the current result is
+// excluded and the returned row reflects the prior state. This is required
+// because SaveResults commits the current run's status before this lookup.
+func LatestCheckStatus(ctx context.Context, checkID string, before time.Time) (*models.CheckStatus, error) {
 	if checkID == "" || uuid.Nil.String() == checkID {
 		return nil, nil
 	}
 	var status models.CheckStatus
-	if err := ctx.DB().Limit(1).Select("time, created_at, status").Where("check_id = ?", checkID).Order("time DESC").Find(&status).Error; err != nil {
+	err := ctx.DB().Select("time, created_at, status").
+		Where("check_id = ? AND time < ?", checkID, before.UTC().Format(time.RFC3339)).
+		Order("time DESC").Take(&status).Error
+	if err != nil {
 		if errors.Is(err, gorm.ErrRecordNotFound) {
 			return nil, nil
 		}

pkg/jobs/canary/canary_jobs.go

@@ -128,12 +128,16 @@ func (j CanaryJob) Run(ctx dutyjob.JobRuntime) error {
 		}
 	}
 
+	// Capture the cutoff before SaveResults so LatestCheckStatus excludes the
+	// current run's just-persisted status and compares against the prior state.
+	now := time.Now()
+
 	transformedChecksCreated, checkIDDeleteStrategyMap, err := SaveResults(ctx.Context, results)
 	if err != nil {
 		return fmt.Errorf("failed to save results: %w", err)
 	}
 
-	UpdateCanaryStatusAndEvent(ctx.Context, j.Canary, results)
+	UpdateCanaryStatusAndEvent(ctx.Context, j.Canary, results, now)
 
 	checkDeleteStrategyGroup := make(map[string][]string)
 	checkIDsToRemove := utils.SetDifference(existingTransformedChecks, transformedChecksCreated)

pkg/jobs/canary/status.go

@@ -17,7 +17,7 @@ import (
 	"k8s.io/apimachinery/pkg/types"
 )
 
-func UpdateCanaryStatusAndEvent(ctx context.Context, canary v1.Canary, results []*pkg.CheckResult) {
+func UpdateCanaryStatusAndEvent(ctx context.Context, canary v1.Canary, results []*pkg.CheckResult, now time.Time) {
 	if CanaryStatusChannel == nil {
 		return
 	}
@@ -40,7 +40,6 @@ func UpdateCanaryStatusAndEvent(ctx context.Context, canary v1.Canary, results [
 	var highestLatency float64
 	var uptimeAgg dutyTypes.Uptime
 
-	transitioned := false
 	for _, result := range results {
 		// Increment duration
 		duration += result.Duration
@@ -62,26 +61,22 @@ func UpdateCanaryStatusAndEvent(ctx context.Context, canary v1.Canary, results [
 			highestLatency = latency.Rolling1H
 		}
 
+		transitioned := false
 		// Transition
 		// q := query.CheckQueryParams{Check: checkID, StatusCount: 1}
 		// if canary.Status.LastTransitionedTime != nil {
 		// 	q.Start = canary.Status.LastTransitionedTime.Format(time.RFC3339)
 		// }
 
-		latestCheckStatus, err := db.LatestCheckStatus(ctx, checkID)
+		latestCheckStatus, err := db.LatestCheckStatus(ctx, checkID, now)
 		if err != nil || latestCheckStatus == nil {
 			transitioned = true
 		} else if latestCheckStatus.Status != result.Pass {
 			transitioned = true
 		}
 		if transitioned {
-			transitionTime := time.Now()
-			if latestCheckStatus != nil {
-				transitionTime = latestCheckStatus.CreatedAt
-			}
-
-			checkStatus[checkID].LastTransitionedTime = &metav1.Time{Time: transitionTime}
-			lastTransitionedTime = &metav1.Time{Time: transitionTime}
+			checkStatus[checkID].LastTransitionedTime = &metav1.Time{Time: now}
+			lastTransitionedTime = &metav1.Time{Time: now}
 		}
 
 		if result.Message != "" {