From c816e36bc11134a5854e8b9244b2f65b3dcb4505 Mon Sep 17 00:00:00 2001 From: Chai Bot Date: Tue, 18 Aug 2026 18:22:42 +0000 Subject: [PATCH 01/10] TRT-2895: Force close regressions Generic tests (e.g. "install should succeed") stay open for weeks because the 5-day regression reuse window reopens recently closed regressions for unrelated failures, causing false "pants on fire" / "failed fix" status. This adds the ability to force close the regressions a resolved triage covered so they are excluded from the reuse window and never reopened. - Store all force close metadata on TestRegression (force_closed, force_closed_by, force_closed_reason, force_closed_by_triage_id) via migration 000013 so a regression is self-contained; no columns are added to triages. - ForceCloseRegressions requires a resolved triage and only closes regressions that existed at the resolution time (opened <= resolved) and are still open, closing them at the resolution time and recording who/why on each regression. It is idempotent and returns ErrTriageNotResolved for unresolved triages. - Exclude force closed regressions from ListCurrentRegressionsForRelease and ResolveTriages so they are not reused. - Add a failure gap query (last failure before / first failure after the resolution) and a dry-run preview endpoint, GET /api/component_readiness/triages/{id}/force_close_preview. - Register POST /api/component_readiness/triages/{id}/force_close_regressions; the handler returns 400 for an unresolved triage. - Expose force_closed / force_closed_by / force_closed_reason / force_closed_by_triage_id directly on the regression detail endpoint. - Document both endpoints in pkg/api/README.md and add unit and e2e tests. Co-Authored-By: Claude Opus 4.8 --- pkg/api/README.md | 62 +++++ .../componentreadiness/regressiontracker.go | 193 +++++++++++++- ...13_add_force_close_to_regressions.down.sql | 7 + ...0013_add_force_close_to_regressions.up.sql | 20 ++ pkg/db/migrations/MANIFEST | 1 + pkg/db/models/triage.go | 15 ++ pkg/sippyserver/server.go | 80 ++++++ .../regressiontracker_test.go | 236 ++++++++++++++++++ .../triage/triageapi_test.go | 180 +++++++++++++ 9 files changed, 792 insertions(+), 2 deletions(-) create mode 100644 pkg/db/migrations/000013_add_force_close_to_regressions.down.sql create mode 100644 pkg/db/migrations/000013_add_force_close_to_regressions.up.sql diff --git a/pkg/api/README.md b/pkg/api/README.md index 35cf16475a..2e39ff6e63 100644 --- a/pkg/api/README.md +++ b/pkg/api/README.md @@ -644,3 +644,65 @@ Updates an existing triage record. Endpoint: `DELETE /api/component_readiness/triages/{id}` Deletes a triage record. + +Endpoint: `POST /api/component_readiness/triages/{id}/force_close_regressions` + +Force closes the open regressions associated with a resolved triage that existed at its +resolution time (opened at or before `resolved`). Force closed regressions are excluded from +the regression reuse window (regressionHysteresisDays), so they are not reopened for unrelated +failures. This prevents generic tests (for example "install should succeed") from staying open +for weeks with false "pants on fire" or "failed fix" status. + +Each regression is closed at the triage's resolution time and records, directly on the +regression row, that it was force closed, by which user, for what reason, and the triage that +drove the action. The operation is idempotent: regressions that opened after the resolution +time, or that are already closed, are left untouched. + +The triage must be resolved. If it is not, the endpoint returns `400 Bad Request` with the +message "Cannot force-close regressions for an unresolved triage. Resolve the triage first." +This is a write endpoint and requires the `write_endpoints` capability. + +### Request body + +| Field | Type | Description | Required | +|--------|--------|----------------------------------------------------------|----------| +| reason | String | The reason the regressions are being force closed. | Yes | + +### Response + +| Field | Type | Description | +|-------------------------|-----------------|-----------------------------------------------------------------| +| closed_regression_ids | Array of number | IDs of the regressions that were open and got closed. | +| timestamp | String (time) | The closed time applied to the regressions (the resolution time). | + +The regression record returned by `GET /api/component_readiness/regressions/{id}` includes +`force_closed`, `force_closed_by`, `force_closed_reason`, and `force_closed_by_triage_id` +directly (no join is required, the data is stored on the regression). + +Endpoint: `GET /api/component_readiness/triages/{id}/force_close_preview` + +Previews (dry run) what `force_close_regressions` would do for a resolved triage, without +modifying anything. Use it to review which regressions would close and to spot any that kept +failing after the claimed resolution before committing. The triage must be resolved; otherwise +the endpoint returns `400 Bad Request` with the same message as the force close endpoint. + +### Response + +| Field | Type | Description | +|-----------------|-----------------|---------------------------------------------------------------------| +| triage_id | Number | The triage being previewed. | +| resolved | String (time) | The triage's resolution time (the cutoff used for scoping). | +| would_close | Array of object | Open regressions that existed at the resolution time (would close). | +| would_not_close | Array of object | Regressions that opened after the resolution time (left untouched). | + +Each regression object in `would_close` / `would_not_close` includes: + +| Field | Type | Description | +|-------------------------------|-----------------|-----------------------------------------------------------------| +| regression_id | Number | The regression ID. | +| test_name | String | The regressed test name. | +| variants | Array of string | The regression's variants. | +| opened | String (time) | When the regression opened. | +| closed | String (time) | When the regression closed, if already closed. | +| last_failure_before_resolution| String (time) | Most recent failing job run at or before the resolution time. | +| first_failure_after_resolution| String (time) | Earliest failing job run after the resolution time, if any (a gap indicator that the test kept failing). | diff --git a/pkg/api/componentreadiness/regressiontracker.go b/pkg/api/componentreadiness/regressiontracker.go index 81a4982ea9..775eadf229 100644 --- a/pkg/api/componentreadiness/regressiontracker.go +++ b/pkg/api/componentreadiness/regressiontracker.go @@ -3,6 +3,7 @@ package componentreadiness import ( "context" "database/sql" + "errors" "fmt" "time" @@ -16,6 +17,7 @@ import ( "github.com/openshift/sippy/pkg/db" "github.com/openshift/sippy/pkg/db/models" log "github.com/sirupsen/logrus" + "gorm.io/gorm" "k8s.io/apimachinery/pkg/util/sets" ) @@ -36,6 +38,14 @@ type RegressionStore interface { UpdateRegression(reg *models.TestRegression) error // ResolveTriages sets the resolution time on any triages that no longer have active regressions ResolveTriages() error + // ForceCloseRegressions closes the open regressions associated with the given resolved triage that + // existed at its resolution time, marking them force closed so they are excluded from the reuse window + // and not reopened for unrelated failures. Returns ErrTriageNotResolved if the triage is not resolved. + // It is idempotent. + ForceCloseRegressions(triageID uint, closedBy, reason string) (*ForceCloseResult, error) + // ForceClosePreview returns a dry-run of what ForceCloseRegressions would do for the given triage, + // without modifying anything. Returns ErrTriageNotResolved if the triage is not resolved. + ForceClosePreview(triageID uint) (*ForceClosePreview, error) // MergeJobRuns upserts job runs for a regression, adding new ones and skipping duplicates. MergeJobRuns(regressionID uint, jobRuns []models.RegressionJobRun) error // UpsertRegressionView records that a regression was observed in a view, setting active=true. @@ -59,9 +69,11 @@ func (prs *PostgresRegressionStore) ListCurrentRegressionsForRelease(release str // List open regressions (no closed date), or those that closed within the last few days. This is to prevent flapping // and return more accurate opened dates when a test is falling in / out of the report. regressions := make([]*models.TestRegression, 0) + // Force closed regressions are excluded from the reuse window even if they closed recently, so they + // are not reopened for unrelated failures (TRT-2895). q := prs.dbc.DB.Table(testRegressionsTable). Where("release = ?", release). - Where("closed IS NULL OR closed > ?", time.Now().Add(-regressionHysteresisDays*24*time.Hour)) + Where("closed IS NULL OR (closed > ? AND force_closed = false)", time.Now().Add(-regressionHysteresisDays*24*time.Hour)) res := q.Scan(®ressions) return regressions, res.Error } @@ -217,7 +229,7 @@ func (prs *PostgresRegressionStore) ResolveTriages() error { subQuery := prs.dbc.DB.Table("triage_regressions tr"). Joins("JOIN test_regressions r ON tr.test_regression_id = r.id"). Where("tr.triage_id = triages.id"). - Where("r.closed IS NULL OR r.closed > ?", hysteresisTime). + Where("r.closed IS NULL OR (r.closed > ? AND r.force_closed = false)", hysteresisTime). Select("1") res := prs.dbc.DB.Table("triages"). @@ -265,6 +277,183 @@ func (prs *PostgresRegressionStore) ResolveTriages() error { return nil } +// ErrTriageNotResolved is returned by force close operations when the triage has not been resolved. +// Force closing needs a resolution time to scope which regressions to close and when to close them. +var ErrTriageNotResolved = errors.New("triage is not resolved") + +// ForceCloseResult summarizes the outcome of a force close operation. It is returned by the API +// so callers know which regressions were closed and at what time. +type ForceCloseResult struct { + // ClosedRegressionIDs are the IDs of the regressions that were open and got closed by this call. + // On an idempotent repeat call (regressions already closed) this will be empty. + ClosedRegressionIDs []uint `json:"closed_regression_ids"` + // Timestamp is the closed time applied to the regressions (the triage's resolution time). + Timestamp time.Time `json:"timestamp"` +} + +// RegressionFailureGap describes the failure timing around a triage's resolution for a single regression. +// A non-nil FirstFailureAfterResolution means the test kept failing after the claimed resolution, a signal +// the triage may have been resolved prematurely and force closing warrants a closer look. +type RegressionFailureGap struct { + // LastFailureBeforeResolution is the most recent failing job run at or before the resolution time. + LastFailureBeforeResolution *time.Time `json:"last_failure_before_resolution,omitempty"` + // FirstFailureAfterResolution is the earliest failing job run after the resolution time, if any. + FirstFailureAfterResolution *time.Time `json:"first_failure_after_resolution,omitempty"` +} + +// ForceClosePreviewRegression describes a single regression in a force close preview, including the failure +// gap around the triage's resolution so a user can judge whether force closing is appropriate. +type ForceClosePreviewRegression struct { + RegressionID uint `json:"regression_id"` + TestName string `json:"test_name"` + Variants pq.StringArray `json:"variants"` + Opened time.Time `json:"opened"` + // Closed is set when the regression is already closed. + Closed *time.Time `json:"closed,omitempty"` + // RegressionFailureGap is embedded so its fields are promoted into this object's JSON. + RegressionFailureGap +} + +// ForceClosePreview is the dry-run result for force closing a triage's regressions. WouldClose lists the +// open regressions that existed at the resolution time and would be closed; WouldNotClose lists regressions +// that opened after the resolution time and would be left untouched. +type ForceClosePreview struct { + TriageID uint `json:"triage_id"` + Resolved time.Time `json:"resolved"` + WouldClose []ForceClosePreviewRegression `json:"would_close"` + WouldNotClose []ForceClosePreviewRegression `json:"would_not_close"` +} + +// ForceCloseRegressions closes the open regressions associated with the given resolved triage that existed +// at its resolution time (opened at or before triage.Resolved), marking them force closed so they are +// excluded from the regression reuse window (regressionHysteresisDays) and never reopened for unrelated +// failures (TRT-2895). Each regression is closed at the triage's resolution time and records who force +// closed it and why, directly on the regression row. The triage must be resolved; otherwise +// ErrTriageNotResolved is returned. It is idempotent: already-closed regressions are left untouched. +func (prs *PostgresRegressionStore) ForceCloseRegressions(triageID uint, closedBy, reason string) (*ForceCloseResult, error) { + result := &ForceCloseResult{} + + err := prs.dbc.DB.Transaction(func(tx *gorm.DB) error { + var triage models.Triage + if err := tx.Preload("Regressions").First(&triage, triageID).Error; err != nil { + return fmt.Errorf("error loading triage %d for force close: %w", triageID, err) + } + if !triage.Resolved.Valid { + return ErrTriageNotResolved + } + closeTime := triage.Resolved.Time + result.Timestamp = closeTime + + for i := range triage.Regressions { + reg := &triage.Regressions[i] + // Only close regressions that existed at the resolution time and are still open. This scopes the + // action to what the triage actually resolved, keeps it idempotent, and preserves the original + // closed time of any already-closed regression. + if reg.Closed.Valid || reg.Opened.After(closeTime) { + continue + } + // Update only the affected columns to avoid rewriting the many2many triage associations. + updates := map[string]interface{}{ + "closed": sql.NullTime{Valid: true, Time: closeTime}, + "force_closed": true, + "force_closed_by": closedBy, + "force_closed_reason": reason, + "force_closed_by_triage_id": triageID, + } + if err := tx.Model(&models.TestRegression{}).Where("id = ?", reg.ID).Updates(updates).Error; err != nil { + return fmt.Errorf("error force closing regression %d: %w", reg.ID, err) + } + result.ClosedRegressionIDs = append(result.ClosedRegressionIDs, reg.ID) + } + return nil + }) + if err != nil { + return nil, err + } + + log.WithField("triageID", triageID).WithField("closedBy", closedBy). + WithField("closedRegressions", len(result.ClosedRegressionIDs)).Info("force closed regressions for triage") + return result, nil +} + +// queryRegressionFailureGap computes, for a single regression, the last failing job run at or before +// resolutionTime and the first failing job run after resolutionTime using the regression_job_runs table. +// Either bound may be nil when no matching failing run exists. +func (prs *PostgresRegressionStore) queryRegressionFailureGap(regressionID uint, resolutionTime time.Time) (RegressionFailureGap, error) { + gap := RegressionFailureGap{} + + var lastBefore sql.NullTime + lastRow := prs.dbc.DB.Table("regression_job_runs"). + Where("regression_id = ? AND start_time <= ? AND test_failed = true", regressionID, resolutionTime). + Select("MAX(start_time)").Row() + if err := lastRow.Scan(&lastBefore); err != nil { + return gap, fmt.Errorf("error querying last failure before resolution for regression %d: %w", regressionID, err) + } + if lastBefore.Valid { + t := lastBefore.Time + gap.LastFailureBeforeResolution = &t + } + + var firstAfter sql.NullTime + firstRow := prs.dbc.DB.Table("regression_job_runs"). + Where("regression_id = ? AND start_time > ? AND test_failed = true", regressionID, resolutionTime). + Select("MIN(start_time)").Row() + if err := firstRow.Scan(&firstAfter); err != nil { + return gap, fmt.Errorf("error querying first failure after resolution for regression %d: %w", regressionID, err) + } + if firstAfter.Valid { + t := firstAfter.Time + gap.FirstFailureAfterResolution = &t + } + + return gap, nil +} + +// ForceClosePreview returns a dry-run of what ForceCloseRegressions would do for the given triage without +// modifying anything. The triage must be resolved; otherwise ErrTriageNotResolved is returned. Each entry +// includes the failure gap around the resolution time so callers can spot regressions that kept failing. +func (prs *PostgresRegressionStore) ForceClosePreview(triageID uint) (*ForceClosePreview, error) { + var triage models.Triage + if err := prs.dbc.DB.Preload("Regressions").First(&triage, triageID).Error; err != nil { + return nil, fmt.Errorf("error loading triage %d for force close preview: %w", triageID, err) + } + if !triage.Resolved.Valid { + return nil, ErrTriageNotResolved + } + resolved := triage.Resolved.Time + preview := &ForceClosePreview{TriageID: triageID, Resolved: resolved} + + for i := range triage.Regressions { + reg := &triage.Regressions[i] + gap, err := prs.queryRegressionFailureGap(reg.ID, resolved) + if err != nil { + return nil, err + } + entry := ForceClosePreviewRegression{ + RegressionID: reg.ID, + TestName: reg.TestName, + Variants: reg.Variants, + Opened: reg.Opened, + RegressionFailureGap: gap, + } + if reg.Closed.Valid { + c := reg.Closed.Time + entry.Closed = &c + } + switch { + case !reg.Closed.Valid && !reg.Opened.After(resolved): + // Open and existed at the resolution time: this is what force close would close. + preview.WouldClose = append(preview.WouldClose, entry) + case reg.Opened.After(resolved): + // Opened after the resolution time: force close would leave it untouched. + preview.WouldNotClose = append(preview.WouldNotClose, entry) + } + // Already-closed regressions that opened before resolution are omitted: force closing would not + // change them. + } + return preview, nil +} + // SyncRegressionsForReport compares regressed tests from a component report against known // regressions in the database, opening new ones, reopening recently closed ones, and updating // stats on existing ones. Returns the list of active regressions after sync. diff --git a/pkg/db/migrations/000013_add_force_close_to_regressions.down.sql b/pkg/db/migrations/000013_add_force_close_to_regressions.down.sql new file mode 100644 index 0000000000..d893d62266 --- /dev/null +++ b/pkg/db/migrations/000013_add_force_close_to_regressions.down.sql @@ -0,0 +1,7 @@ +DROP INDEX IF EXISTS idx_test_regressions_force_closed_by_triage_id; + +ALTER TABLE test_regressions + DROP COLUMN IF EXISTS force_closed_by_triage_id, + DROP COLUMN IF EXISTS force_closed_reason, + DROP COLUMN IF EXISTS force_closed_by, + DROP COLUMN IF EXISTS force_closed; diff --git a/pkg/db/migrations/000013_add_force_close_to_regressions.up.sql b/pkg/db/migrations/000013_add_force_close_to_regressions.up.sql new file mode 100644 index 0000000000..59c83765c8 --- /dev/null +++ b/pkg/db/migrations/000013_add_force_close_to_regressions.up.sql @@ -0,0 +1,20 @@ +-- TRT-2895: Force close regressions. +-- +-- Generic tests (e.g. "install should succeed") stay open for weeks because the +-- 5-day regression reuse window (regressionHysteresisDays) reopens recently +-- closed regressions for unrelated failures, causing false "pants on fire" / +-- "failed fix" status. Force closing a resolved triage's regressions marks them +-- so they are excluded from the reuse window and never reopened. +-- +-- All force close metadata lives on test_regressions so a regression is +-- self-contained: it records that it was force closed, by whom, why, and which +-- triage drove the action. Existing rows default to not force closed. + +ALTER TABLE test_regressions + ADD COLUMN IF NOT EXISTS force_closed BOOLEAN NOT NULL DEFAULT false, + ADD COLUMN IF NOT EXISTS force_closed_by TEXT NOT NULL DEFAULT '', + ADD COLUMN IF NOT EXISTS force_closed_reason TEXT NOT NULL DEFAULT '', + ADD COLUMN IF NOT EXISTS force_closed_by_triage_id BIGINT; + +CREATE INDEX IF NOT EXISTS idx_test_regressions_force_closed_by_triage_id + ON test_regressions (force_closed_by_triage_id); diff --git a/pkg/db/migrations/MANIFEST b/pkg/db/migrations/MANIFEST index 0c6d1c812e..ae21f9d0bd 100644 --- a/pkg/db/migrations/MANIFEST +++ b/pkg/db/migrations/MANIFEST @@ -19,3 +19,4 @@ 000010_drop_test_analysis_by_job_by_dates 000011_add_lifecycle_to_summaries 000012_drop_test_daily_totals_date_index +000013_add_force_close_to_regressions diff --git a/pkg/db/models/triage.go b/pkg/db/models/triage.go index d56e7a179c..af7fd8e6f3 100644 --- a/pkg/db/models/triage.go +++ b/pkg/db/models/triage.go @@ -229,6 +229,21 @@ type TestRegression struct { // disappear on their own. MaxFailures int `json:"max_failures"` + // ForceClosed indicates this regression was force closed via a resolved triage. Force closed regressions + // are excluded from the regression reuse window (regressionHysteresisDays) so they are not reopened for + // unrelated failures (TRT-2895). All force close metadata is stored directly on the regression so the + // record is self-contained and needs no join back to the triage. + ForceClosed bool `json:"force_closed" gorm:"not null;default:false"` + + // ForceClosedBy records the user who force closed this regression. + ForceClosedBy string `json:"force_closed_by" gorm:"default:''"` + + // ForceClosedReason records the user supplied reason for force closing this regression. + ForceClosedReason string `json:"force_closed_reason" gorm:"default:''"` + + // ForceClosedByTriageID references the triage whose resolution drove the force close, if any. + ForceClosedByTriageID *uint `json:"force_closed_by_triage_id" gorm:"index"` + // JobRuns accumulates the unique set of all job runs ever observed while this regression was open. // As the 7-day sample window slides, old runs roll off and new ones appear, but this list retains all of them. JobRuns []RegressionJobRun `json:"job_runs,omitempty" gorm:"foreignKey:RegressionID;constraint:OnDelete:CASCADE;"` diff --git a/pkg/sippyserver/server.go b/pkg/sippyserver/server.go index 2b04a63f35..7665d4d246 100644 --- a/pkg/sippyserver/server.go +++ b/pkg/sippyserver/server.go @@ -1966,6 +1966,72 @@ func (s *Server) jsonUpdateTriage(w http.ResponseWriter, req *http.Request) { api.RespondWithJSON(http.StatusOK, w, triage) } +// forceCloseRegressionsRequest is the request body for the force close regressions endpoint. +type forceCloseRegressionsRequest struct { + // Reason is a required user supplied explanation for force closing the triage's regressions. + Reason string `json:"reason"` +} + +func (s *Server) jsonForceCloseRegressions(w http.ResponseWriter, req *http.Request) { + vars := mux.Vars(req) + idStr := vars["id"] + triageID, err := strconv.Atoi(idStr) + if err != nil { + failureResponse(w, http.StatusBadRequest, "invalid ID format: "+idStr) + return + } + + user := getUserForRequest(req) + log.Infof("triage force_close_regressions POST made by user: %s", user) + + var forceCloseReq forceCloseRegressionsRequest + if err := json.NewDecoder(req.Body).Decode(&forceCloseReq); err != nil { + log.WithError(err).Error("error parsing force close regressions request") + failureResponse(w, http.StatusBadRequest, err.Error()) + return + } + if strings.TrimSpace(forceCloseReq.Reason) == "" { + failureResponse(w, http.StatusBadRequest, "reason is required to force close regressions") + return + } + + tracker := componentreadiness.NewPostgresRegressionStore(s.db, s.jiraClient) + result, err := tracker.ForceCloseRegressions(uint(triageID), user, forceCloseReq.Reason) // nolint:gosec + if err != nil { + if errors.Is(err, componentreadiness.ErrTriageNotResolved) { + failureResponse(w, http.StatusBadRequest, "Cannot force-close regressions for an unresolved triage. Resolve the triage first.") + return + } + log.WithError(err).Error("error force closing regressions") + failureResponse(w, http.StatusInternalServerError, err.Error()) + return + } + api.RespondWithJSON(http.StatusOK, w, result) +} + +func (s *Server) jsonForceClosePreview(w http.ResponseWriter, req *http.Request) { + vars := mux.Vars(req) + idStr := vars["id"] + triageID, err := strconv.Atoi(idStr) + if err != nil { + failureResponse(w, http.StatusBadRequest, "invalid ID format: "+idStr) + return + } + + tracker := componentreadiness.NewPostgresRegressionStore(s.db, s.jiraClient) + preview, err := tracker.ForceClosePreview(uint(triageID)) // nolint:gosec + if err != nil { + if errors.Is(err, componentreadiness.ErrTriageNotResolved) { + failureResponse(w, http.StatusBadRequest, "Cannot force-close regressions for an unresolved triage. Resolve the triage first.") + return + } + log.WithError(err).Error("error building force close preview") + failureResponse(w, http.StatusInternalServerError, err.Error()) + return + } + api.RespondWithJSON(http.StatusOK, w, preview) +} + func (s *Server) jsonDeleteTriage(w http.ResponseWriter, req *http.Request) { vars := mux.Vars(req) idStr := vars["id"] @@ -2876,6 +2942,20 @@ func (s *Server) Serve() { Capabilities: []string{LocalDBCapability, ComponentReadinessCapability, WriteEndpointsCapability}, HandlerFunc: s.jsonDeleteTriage, }, + { + EndpointPath: "/api/component_readiness/triages/{id}/force_close_regressions", + Description: "Force close the open regressions that existed at a resolved triage's resolution time so they are excluded from the regression reuse window", + Methods: []string{http.MethodPost}, + Capabilities: []string{LocalDBCapability, ComponentReadinessCapability, WriteEndpointsCapability}, + HandlerFunc: s.jsonForceCloseRegressions, + }, + { + EndpointPath: "/api/component_readiness/triages/{id}/force_close_preview", + Description: "Preview which regressions would be force closed for a resolved triage, with failure gap data, without modifying anything", + Methods: []string{http.MethodGet}, + Capabilities: []string{LocalDBCapability, ComponentReadinessCapability}, + HandlerFunc: s.jsonForceClosePreview, + }, { EndpointPath: "/api/component_readiness/triages/{id}/matches", Description: "List potential matching regressions for a given triage.", diff --git a/test/e2e/componentreadiness/regressiontracker/regressiontracker_test.go b/test/e2e/componentreadiness/regressiontracker/regressiontracker_test.go index 6a9debe423..d99601475d 100644 --- a/test/e2e/componentreadiness/regressiontracker/regressiontracker_test.go +++ b/test/e2e/componentreadiness/regressiontracker/regressiontracker_test.go @@ -273,6 +273,242 @@ func Test_RegressionTracker(t *testing.T) { } +func Test_ForceCloseRegressions(t *testing.T) { + dbc := util.CreateE2EPostgresConnection(t) + tracker := componentreadiness.NewPostgresRegressionStore(dbc, nil) + rLog := log.WithField("test", "force-close-regressions") + + forceView := crview.View{ + Name: "4.19-main", + SampleRelease: reqopts.RelativeRelease{ + Release: reqopts.Release{Name: "4.19"}, + }, + BaseRelease: reqopts.RelativeRelease{ + Release: reqopts.Release{Name: "4.18"}, + }, + } + + newRegSummary := func(testID string) componentreport.ReportTestSummary { + return componentreport.ReportTestSummary{ + TestComparison: testdetails.TestComparison{ + BaseStats: &testdetails.ReleaseStats{Release: "4.18"}, + }, + Identification: crtest.Identification{ + RowIdentification: crtest.RowIdentification{ + Component: "comp", + Capability: "cap", + TestName: "force close test " + testID, + TestID: testID, + }, + ColumnIdentification: crtest.ColumnIdentification{ + Variants: map[string]string{"a": "b"}, + }, + }, + } + } + + makeReport := func(tests ...componentreport.ReportTestSummary) *componentreport.ComponentReport { + return &componentreport.ComponentReport{ + Rows: []componentreport.ReportRow{ + {Columns: []componentreport.ReportColumn{{RegressedTests: tests}}}, + }, + } + } + + createTriageForRegressions := func(t *testing.T, url string, regs ...*models.TestRegression) models.Triage { + t.Helper() + regressions := make([]models.TestRegression, len(regs)) + for i, r := range regs { + regressions[i] = models.TestRegression{ID: r.ID} + } + triage := models.Triage{ + URL: url, + Description: "force close triage", + Type: models.TriageTypeProduct, + Regressions: regressions, + } + dbWithContext := dbc.DB.WithContext(context.WithValue(context.Background(), models.CurrentUserKey, "e2e-test")) + require.NoError(t, dbWithContext.Create(&triage).Error) + return triage + } + + createResolvedTriageForRegressions := func(t *testing.T, url string, resolved time.Time, regs ...*models.TestRegression) models.Triage { + t.Helper() + triage := createTriageForRegressions(t, url, regs...) + dbWithContext := dbc.DB.WithContext(context.WithValue(context.Background(), models.CurrentUserKey, "e2e-test")) + require.NoError(t, dbWithContext.Model(&triage).Update("resolved", sql.NullTime{Valid: true, Time: resolved}).Error) + return triage + } + + cleanup := func() { + cleanupTriages(dbc) + cleanupAllRegressions(dbc) + } + + t.Run("records force close details on each regression", func(t *testing.T) { + defer cleanup() + + resolved := time.Now().Truncate(time.Second) + reg, err := rawCreateRegression(dbc, "4.19", "fc-fields", "force close test fc-fields", + []string{"a:b"}, resolved.Add(-10*24*time.Hour), time.Time{}) + require.NoError(t, err) + triage := createResolvedTriageForRegressions(t, "https://redhat.atlassian.net/browse/TEST-FC-1", resolved, reg) + + result, err := tracker.ForceCloseRegressions(triage.ID, "developer", "generic test, unrelated failures") + require.NoError(t, err) + require.NotNil(t, result) + assert.ElementsMatch(t, []uint{reg.ID}, result.ClosedRegressionIDs) + assert.WithinDuration(t, resolved, result.Timestamp, time.Second, "timestamp should be the resolution time") + + var checkReg models.TestRegression + require.NoError(t, dbc.DB.First(&checkReg, reg.ID).Error) + assert.True(t, checkReg.Closed.Valid, "regression should be closed") + assert.WithinDuration(t, resolved, checkReg.Closed.Time, time.Second, "regression should close at the resolution time") + assert.True(t, checkReg.ForceClosed, "regression should be force closed") + assert.Equal(t, "developer", checkReg.ForceClosedBy, "regression should record who force closed it") + assert.Equal(t, "generic test, unrelated failures", checkReg.ForceClosedReason, "regression should record the reason") + require.NotNil(t, checkReg.ForceClosedByTriageID) + assert.Equal(t, triage.ID, *checkReg.ForceClosedByTriageID) + }) + + t.Run("returns ErrTriageNotResolved for an unresolved triage", func(t *testing.T) { + defer cleanup() + + reg, err := rawCreateRegression(dbc, "4.19", "fc-unresolved", "force close test fc-unresolved", + []string{"a:b"}, time.Now().Add(-10*24*time.Hour), time.Time{}) + require.NoError(t, err) + triage := createTriageForRegressions(t, "https://redhat.atlassian.net/browse/TEST-FC-UNRESOLVED", reg) + + _, err = tracker.ForceCloseRegressions(triage.ID, "developer", "should fail") + require.Error(t, err) + assert.ErrorIs(t, err, componentreadiness.ErrTriageNotResolved) + + // The regression must remain untouched. + var checkReg models.TestRegression + require.NoError(t, dbc.DB.First(&checkReg, reg.ID).Error) + assert.False(t, checkReg.Closed.Valid, "regression should remain open") + assert.False(t, checkReg.ForceClosed, "regression should not be force closed") + }) + + t.Run("only closes regressions that existed at the resolution time", func(t *testing.T) { + defer cleanup() + + resolved := time.Now().Add(-5 * 24 * time.Hour).Truncate(time.Second) + before, err := rawCreateRegression(dbc, "4.19", "fc-before", "force close test fc-before", + []string{"a:b"}, resolved.Add(-5*24*time.Hour), time.Time{}) + require.NoError(t, err) + after, err := rawCreateRegression(dbc, "4.19", "fc-after", "force close test fc-after", + []string{"a:b"}, resolved.Add(24*time.Hour), time.Time{}) + require.NoError(t, err) + triage := createResolvedTriageForRegressions(t, "https://redhat.atlassian.net/browse/TEST-FC-SCOPE", resolved, before, after) + + result, err := tracker.ForceCloseRegressions(triage.ID, "developer", "scoped close") + require.NoError(t, err) + assert.ElementsMatch(t, []uint{before.ID}, result.ClosedRegressionIDs, + "only the regression opened at or before the resolution time should be closed") + + var checkBefore models.TestRegression + require.NoError(t, dbc.DB.First(&checkBefore, before.ID).Error) + assert.True(t, checkBefore.ForceClosed, "regression opened before resolution should be force closed") + assert.WithinDuration(t, resolved, checkBefore.Closed.Time, time.Second) + + var checkAfter models.TestRegression + require.NoError(t, dbc.DB.First(&checkAfter, after.ID).Error) + assert.False(t, checkAfter.Closed.Valid, "regression opened after resolution should remain open") + assert.False(t, checkAfter.ForceClosed, "regression opened after resolution should not be force closed") + }) + + t.Run("is idempotent", func(t *testing.T) { + defer cleanup() + + resolved := time.Now().Truncate(time.Second) + reg, err := rawCreateRegression(dbc, "4.19", "fc-idempotent", "force close test fc-idempotent", + []string{"a:b"}, resolved.Add(-10*24*time.Hour), time.Time{}) + require.NoError(t, err) + triage := createResolvedTriageForRegressions(t, "https://redhat.atlassian.net/browse/TEST-FC-3", resolved, reg) + + result1, err := tracker.ForceCloseRegressions(triage.ID, "developer", "first call") + require.NoError(t, err) + require.Len(t, result1.ClosedRegressionIDs, 1) + + var closedReg models.TestRegression + require.NoError(t, dbc.DB.First(&closedReg, reg.ID).Error) + originalClosedTime := closedReg.Closed.Time + + // Second call should be a no-op: no open regressions remain. + result2, err := tracker.ForceCloseRegressions(triage.ID, "developer", "first call") + require.NoError(t, err) + assert.Empty(t, result2.ClosedRegressionIDs, "no regressions should be closed on repeat call") + + require.NoError(t, dbc.DB.First(&closedReg, reg.ID).Error) + assert.WithinDuration(t, originalClosedTime, closedReg.Closed.Time, time.Second, + "closed time should not change on repeat call") + }) + + t.Run("force closed regression excluded from ListCurrentRegressionsForRelease", func(t *testing.T) { + defer cleanup() + + resolved := time.Now().Truncate(time.Second) + reg, err := rawCreateRegression(dbc, "4.19", "fc-excluded", "force close test fc-excluded", + []string{"a:b"}, resolved.Add(-24*time.Hour), time.Time{}) + require.NoError(t, err) + triage := createResolvedTriageForRegressions(t, "https://redhat.atlassian.net/browse/TEST-FC-4", resolved, reg) + + // Before force close, the recently opened regression is listed. + before, err := tracker.ListCurrentRegressionsForRelease("4.19") + require.NoError(t, err) + assert.True(t, containsRegression(before, reg.ID), "open regression should be listed before force close") + + _, err = tracker.ForceCloseRegressions(triage.ID, "developer", "exclude from reuse") + require.NoError(t, err) + + // After force close, even though it closed just now (within the hysteresis window), it must be excluded. + after, err := tracker.ListCurrentRegressionsForRelease("4.19") + require.NoError(t, err) + assert.False(t, containsRegression(after, reg.ID), "force closed regression should be excluded from reuse list") + }) + + t.Run("force closed regression is not reused by SyncRegressionsForReport", func(t *testing.T) { + defer cleanup() + + report := makeReport(newRegSummary("fc-not-reused")) + + // First sync opens a regression. + regs1, err := componentreadiness.SyncRegressionsForReport(tracker, forceView, rLog, report) + require.NoError(t, err) + require.Len(t, regs1, 1) + originalID := regs1[0].ID + + // Force close it via a resolved triage. + triage := createResolvedTriageForRegressions(t, "https://redhat.atlassian.net/browse/TEST-FC-5", + time.Now().Add(time.Minute), regs1[0]) + _, err = tracker.ForceCloseRegressions(triage.ID, "developer", "not reused") + require.NoError(t, err) + + // Re-sync the same report: a brand new regression should open, not the force closed one. + regs2, err := componentreadiness.SyncRegressionsForReport(tracker, forceView, rLog, report) + require.NoError(t, err) + require.Len(t, regs2, 1) + assert.NotEqual(t, originalID, regs2[0].ID, + "a new regression should open instead of reusing the force closed one") + + // The original stays closed and force closed. + var original models.TestRegression + require.NoError(t, dbc.DB.First(&original, originalID).Error) + assert.True(t, original.Closed.Valid, "force closed regression should remain closed") + assert.True(t, original.ForceClosed, "force closed regression should remain force closed") + }) +} + +func containsRegression(regressions []*models.TestRegression, id uint) bool { + for _, r := range regressions { + if r.ID == id { + return true + } + } + return false +} + func cleanupJobRuns(dbc *db.DB) { res := dbc.DB.Where("1 = 1").Delete(&models.RegressionJobRun{}) if res.Error != nil { diff --git a/test/e2e/componentreadiness/triage/triageapi_test.go b/test/e2e/componentreadiness/triage/triageapi_test.go index 64cdd65657..28630f6a35 100644 --- a/test/e2e/componentreadiness/triage/triageapi_test.go +++ b/test/e2e/componentreadiness/triage/triageapi_test.go @@ -727,6 +727,186 @@ func Test_TriageAPI(t *testing.T) { } +func Test_ForceCloseRegressionsAPI(t *testing.T) { + dbc := util.CreateE2EPostgresConnection(t) + tracker := componentreadiness.NewPostgresRegressionStore(dbc, nil) + + jiraBug := createBug(t, dbc.DB) + defer dbc.DB.Delete(jiraBug) + + release := view.SampleRelease.Name + + // createRawRegression creates a regression with a caller-controlled opened time so tests can exercise + // the resolution-time scoping. + createRawRegression := func(t *testing.T, testID string, opened time.Time) *models.TestRegression { + t.Helper() + reg := &models.TestRegression{ + Release: release, + TestID: testID, + TestName: "force close api test " + testID, + Variants: pq.StringArray{"a:b"}, + Opened: opened, + } + require.NoError(t, dbc.DB.Create(reg).Error) + return reg + } + + // resolveTriage resolves a triage via the API so force close has a resolution time to scope against. + resolveTriage := func(t *testing.T, triageResp models.Triage, resolved time.Time) { + t.Helper() + triageResp.Resolved = sql.NullTime{Valid: true, Time: resolved} + var updated models.Triage + require.NoError(t, util.SippyPut(fmt.Sprintf("/api/component_readiness/triages/%d", triageResp.ID), &triageResp, &updated)) + } + + t.Run("force close requires a reason", func(t *testing.T) { + defer cleanupAllTriages(dbc) + reg := createTestRegression(t, tracker, view, "fc-api-reason") + defer dbc.DB.Delete(reg) + triageResponse := createAndValidateTriageRecord(t, jiraBug.URL, reg) + resolveTriage(t, triageResponse, time.Now().Add(time.Minute)) + + // Empty reason should be rejected. + var result componentreadiness.ForceCloseResult + err := util.SippyPost(fmt.Sprintf("/api/component_readiness/triages/%d/force_close_regressions", triageResponse.ID), + &map[string]string{"reason": ""}, &result) + require.Error(t, err, "force close with empty reason should fail") + }) + + t.Run("force close on an unresolved triage is rejected", func(t *testing.T) { + defer cleanupAllTriages(dbc) + reg := createTestRegression(t, tracker, view, "fc-api-unresolved") + defer dbc.DB.Delete(reg) + triageResponse := createAndValidateTriageRecord(t, jiraBug.URL, reg) + + // Force closing an unresolved triage must be rejected. + var result componentreadiness.ForceCloseResult + err := util.SippyPost(fmt.Sprintf("/api/component_readiness/triages/%d/force_close_regressions", triageResponse.ID), + &map[string]string{"reason": "should be rejected"}, &result) + require.Error(t, err, "force closing an unresolved triage should fail") + + // Preview of an unresolved triage is likewise rejected. + var preview componentreadiness.ForceClosePreview + err = util.SippyGet(fmt.Sprintf("/api/component_readiness/triages/%d/force_close_preview", triageResponse.ID), &preview) + require.Error(t, err, "previewing an unresolved triage should fail") + }) + + t.Run("force close closes regressions and excludes them from reuse", func(t *testing.T) { + defer cleanupAllTriages(dbc) + reg := createTestRegression(t, tracker, view, "fc-api-close") + defer dbc.DB.Delete(reg) + triageResponse := createAndValidateTriageRecord(t, jiraBug.URL, reg) + resolveTriage(t, triageResponse, time.Now().Add(time.Minute)) + + var result componentreadiness.ForceCloseResult + err := util.SippyPost(fmt.Sprintf("/api/component_readiness/triages/%d/force_close_regressions", triageResponse.ID), + &map[string]string{"reason": "generic test, unrelated failures"}, &result) + require.NoError(t, err) + assert.ElementsMatch(t, []uint{reg.ID}, result.ClosedRegressionIDs) + assert.False(t, result.Timestamp.IsZero()) + + // Regression should be closed and excluded from the reuse list. + regressions, err := tracker.ListCurrentRegressionsForRelease(release) + require.NoError(t, err) + for _, r := range regressions { + assert.NotEqual(t, reg.ID, r.ID, "force closed regression should not appear in reuse list") + } + + // The regression records who force closed it and why, directly on the regression row. + var checkReg models.TestRegression + require.NoError(t, dbc.DB.First(&checkReg, reg.ID).Error) + assert.True(t, checkReg.ForceClosed) + assert.Equal(t, "developer", checkReg.ForceClosedBy) + assert.Equal(t, "generic test, unrelated failures", checkReg.ForceClosedReason) + }) + + t.Run("regression detail exposes force close info", func(t *testing.T) { + defer cleanupAllTriages(dbc) + reg := createTestRegression(t, tracker, view, "fc-api-detail") + defer dbc.DB.Delete(reg) + // Associate with a view so the regression detail endpoint can build HATEOAS links. + require.NoError(t, tracker.UpsertRegressionView(reg.ID, view.Name)) + defer dbc.DB.Where("test_regression_id = ?", reg.ID).Delete(&models.RegressionView{}) + triageResponse := createAndValidateTriageRecord(t, jiraBug.URL, reg) + resolveTriage(t, triageResponse, time.Now().Add(time.Minute)) + + var result componentreadiness.ForceCloseResult + err := util.SippyPost(fmt.Sprintf("/api/component_readiness/triages/%d/force_close_regressions", triageResponse.ID), + &map[string]string{"reason": "detail exposure reason"}, &result) + require.NoError(t, err) + + var detail models.TestRegression + err = util.SippyGet(fmt.Sprintf("/api/component_readiness/regressions/%d", reg.ID), &detail) + require.NoError(t, err) + assert.True(t, detail.ForceClosed, "regression detail should report force_closed") + require.NotNil(t, detail.ForceClosedByTriageID) + assert.Equal(t, triageResponse.ID, *detail.ForceClosedByTriageID) + assert.Equal(t, "developer", detail.ForceClosedBy, "detail should include force_closed_by directly from the regression") + assert.Equal(t, "detail exposure reason", detail.ForceClosedReason, "detail should include force_closed_reason directly from the regression") + }) + + t.Run("force close is idempotent over the API", func(t *testing.T) { + defer cleanupAllTriages(dbc) + reg := createTestRegression(t, tracker, view, "fc-api-idempotent") + defer dbc.DB.Delete(reg) + triageResponse := createAndValidateTriageRecord(t, jiraBug.URL, reg) + resolveTriage(t, triageResponse, time.Now().Add(time.Minute)) + + var result1 componentreadiness.ForceCloseResult + err := util.SippyPost(fmt.Sprintf("/api/component_readiness/triages/%d/force_close_regressions", triageResponse.ID), + &map[string]string{"reason": "idempotent"}, &result1) + require.NoError(t, err) + require.Len(t, result1.ClosedRegressionIDs, 1) + + var result2 componentreadiness.ForceCloseResult + err = util.SippyPost(fmt.Sprintf("/api/component_readiness/triages/%d/force_close_regressions", triageResponse.ID), + &map[string]string{"reason": "idempotent"}, &result2) + require.NoError(t, err) + assert.Empty(t, result2.ClosedRegressionIDs, "repeat force close should close no additional regressions") + }) + + t.Run("preview lists would-close and would-not-close regressions with gap data", func(t *testing.T) { + defer cleanupAllTriages(dbc) + resolved := time.Now().Add(-5 * 24 * time.Hour).Truncate(time.Second) + + wouldClose := createRawRegression(t, "fc-prev-close", resolved.Add(-10*24*time.Hour)) + defer dbc.DB.Delete(wouldClose) + wouldNotClose := createRawRegression(t, "fc-prev-open", resolved.Add(24*time.Hour)) + defer dbc.DB.Delete(wouldNotClose) + + // Failures before and after the resolution time drive the gap indicator. + require.NoError(t, tracker.MergeJobRuns(wouldClose.ID, []models.RegressionJobRun{ + {ProwJobRunID: "prev-before", ProwJobName: "job-1", StartTime: resolved.Add(-2 * 24 * time.Hour), TestFailed: true}, + {ProwJobRunID: "prev-after", ProwJobName: "job-1", StartTime: resolved.Add(2 * 24 * time.Hour), TestFailed: true}, + })) + + triage := models.Triage{ + URL: jiraBug.URL, + Type: models.TriageTypeProduct, + Regressions: []models.TestRegression{ + {ID: wouldClose.ID}, + {ID: wouldNotClose.ID}, + }, + } + var triageResp models.Triage + require.NoError(t, util.SippyPost("/api/component_readiness/triages", &triage, &triageResp)) + resolveTriage(t, triageResp, resolved) + + var preview componentreadiness.ForceClosePreview + require.NoError(t, util.SippyGet(fmt.Sprintf("/api/component_readiness/triages/%d/force_close_preview", triageResp.ID), &preview)) + + require.Len(t, preview.WouldClose, 1, "regression opened before resolution should be in would_close") + assert.Equal(t, wouldClose.ID, preview.WouldClose[0].RegressionID) + require.NotNil(t, preview.WouldClose[0].LastFailureBeforeResolution, "should report last failure before resolution") + assert.WithinDuration(t, resolved.Add(-2*24*time.Hour), *preview.WouldClose[0].LastFailureBeforeResolution, time.Second) + require.NotNil(t, preview.WouldClose[0].FirstFailureAfterResolution, "should report first failure after resolution") + assert.WithinDuration(t, resolved.Add(2*24*time.Hour), *preview.WouldClose[0].FirstFailureAfterResolution, time.Second) + + require.Len(t, preview.WouldNotClose, 1, "regression opened after resolution should be in would_not_close") + assert.Equal(t, wouldNotClose.ID, preview.WouldNotClose[0].RegressionID) + }) +} + func Test_RegressionAPI(t *testing.T) { dbc := util.CreateE2EPostgresConnection(t) // jiraClient is intentionally nil to prevent commenting on jiras From cbf61001f6ed14dbfcbfc9bccc84d009a2cf99d4 Mon Sep 17 00:00:00 2001 From: Chai Bot Date: Wed, 19 Aug 2026 02:25:04 +0000 Subject: [PATCH 02/10] TRT-2895: Address force-close review feedback Adversarial review follow-ups on the force-close regressions feature: - Batch the failure-gap lookups. queryRegressionFailureGap ran two queries per regression inside the preview loop (N+1). Replace it with queryRegressionFailureGaps, which runs a single grouped query per direction using `regression_id IN (...)` and `GROUP BY regression_id`. - Stop using Preload("Regressions") in ForceCloseRegressions and ForceClosePreview, which loaded every regression ever associated with the triage (including old closed ones). Both now join through triage_regressions and filter at the DB level: ForceCloseRegressions selects only open regressions that existed at the resolution time (closed IS NULL AND opened <= resolved) and closes them in a single UPDATE; ForceClosePreview loads only the regressions it reports on. - Reject force close when the requesting user cannot be determined. The handler now returns 401 instead of recording an empty ForceClosedBy. - Add a partial index on test_regressions (force_closed) WHERE force_closed = true so the reuse-window queries that exclude force closed regressions stay cheap; drop it in the down migration. Co-Authored-By: Claude Opus 4.8 --- .../componentreadiness/regressiontracker.go | 161 ++++++++++++------ ...13_add_force_close_to_regressions.down.sql | 1 + ...0013_add_force_close_to_regressions.up.sql | 5 + pkg/sippyserver/server.go | 6 + 4 files changed, 117 insertions(+), 56 deletions(-) diff --git a/pkg/api/componentreadiness/regressiontracker.go b/pkg/api/componentreadiness/regressiontracker.go index 775eadf229..7d4a097a78 100644 --- a/pkg/api/componentreadiness/regressiontracker.go +++ b/pkg/api/componentreadiness/regressiontracker.go @@ -335,7 +335,7 @@ func (prs *PostgresRegressionStore) ForceCloseRegressions(triageID uint, closedB err := prs.dbc.DB.Transaction(func(tx *gorm.DB) error { var triage models.Triage - if err := tx.Preload("Regressions").First(&triage, triageID).Error; err != nil { + if err := tx.First(&triage, triageID).Error; err != nil { return fmt.Errorf("error loading triage %d for force close: %w", triageID, err) } if !triage.Resolved.Valid { @@ -344,27 +344,36 @@ func (prs *PostgresRegressionStore) ForceCloseRegressions(triageID uint, closedB closeTime := triage.Resolved.Time result.Timestamp = closeTime - for i := range triage.Regressions { - reg := &triage.Regressions[i] - // Only close regressions that existed at the resolution time and are still open. This scopes the - // action to what the triage actually resolved, keeps it idempotent, and preserves the original - // closed time of any already-closed regression. - if reg.Closed.Valid || reg.Opened.After(closeTime) { - continue - } - // Update only the affected columns to avoid rewriting the many2many triage associations. - updates := map[string]interface{}{ - "closed": sql.NullTime{Valid: true, Time: closeTime}, - "force_closed": true, - "force_closed_by": closedBy, - "force_closed_reason": reason, - "force_closed_by_triage_id": triageID, - } - if err := tx.Model(&models.TestRegression{}).Where("id = ?", reg.ID).Updates(updates).Error; err != nil { - return fmt.Errorf("error force closing regression %d: %w", reg.ID, err) - } - result.ClosedRegressionIDs = append(result.ClosedRegressionIDs, reg.ID) + // Select only the regressions this triage actually resolved: those that existed at the + // resolution time (opened <= closeTime) and are still open (closed IS NULL). Filtering in the + // query rather than loading every regression ever associated with the triage keeps the action + // scoped, keeps it idempotent, and preserves the closed time of already-closed regressions. + var regIDsToClose []uint + if err := tx.Table(testRegressionsTable). + Joins("JOIN triage_regressions ON triage_regressions.test_regression_id = test_regressions.id"). + Where("triage_regressions.triage_id = ?", triageID). + Where("test_regressions.closed IS NULL"). + Where("test_regressions.opened <= ?", closeTime). + Pluck("test_regressions.id", ®IDsToClose).Error; err != nil { + return fmt.Errorf("error finding regressions to force close for triage %d: %w", triageID, err) + } + if len(regIDsToClose) == 0 { + return nil + } + + // Update only the affected columns in a single statement to avoid rewriting the many2many + // triage associations. + updates := map[string]interface{}{ + "closed": sql.NullTime{Valid: true, Time: closeTime}, + "force_closed": true, + "force_closed_by": closedBy, + "force_closed_reason": reason, + "force_closed_by_triage_id": triageID, + } + if err := tx.Model(&models.TestRegression{}).Where("id IN ?", regIDsToClose).Updates(updates).Error; err != nil { + return fmt.Errorf("error force closing regressions for triage %d: %w", triageID, err) } + result.ClosedRegressionIDs = regIDsToClose return nil }) if err != nil { @@ -376,37 +385,60 @@ func (prs *PostgresRegressionStore) ForceCloseRegressions(triageID uint, closedB return result, nil } -// queryRegressionFailureGap computes, for a single regression, the last failing job run at or before -// resolutionTime and the first failing job run after resolutionTime using the regression_job_runs table. -// Either bound may be nil when no matching failing run exists. -func (prs *PostgresRegressionStore) queryRegressionFailureGap(regressionID uint, resolutionTime time.Time) (RegressionFailureGap, error) { - gap := RegressionFailureGap{} - - var lastBefore sql.NullTime - lastRow := prs.dbc.DB.Table("regression_job_runs"). - Where("regression_id = ? AND start_time <= ? AND test_failed = true", regressionID, resolutionTime). - Select("MAX(start_time)").Row() - if err := lastRow.Scan(&lastBefore); err != nil { - return gap, fmt.Errorf("error querying last failure before resolution for regression %d: %w", regressionID, err) +// queryRegressionFailureGaps computes, for each of the given regressions, the last failing job run at +// or before resolutionTime and the first failing job run after resolutionTime using the +// regression_job_runs table. It runs a single grouped query per direction (rather than a query per +// regression) to avoid an N+1 pattern. Regressions with no matching failing run are absent from the +// corresponding map entry / field. +func (prs *PostgresRegressionStore) queryRegressionFailureGaps(regressionIDs []uint, resolutionTime time.Time) (map[uint]RegressionFailureGap, error) { + gaps := make(map[uint]RegressionFailureGap, len(regressionIDs)) + if len(regressionIDs) == 0 { + return gaps, nil + } + + // gapRow captures the grouped aggregate (MAX/MIN start_time) per regression_id. + type gapRow struct { + RegressionID uint + FailureTime sql.NullTime + } + + // Last failing run at or before the resolution time, one row per regression. + var lastRows []gapRow + if err := prs.dbc.DB.Table("regression_job_runs"). + Select("regression_id, MAX(start_time) AS failure_time"). + Where("regression_id IN ? AND start_time <= ? AND test_failed = true", regressionIDs, resolutionTime). + Group("regression_id"). + Scan(&lastRows).Error; err != nil { + return nil, fmt.Errorf("error querying last failures before resolution: %w", err) } - if lastBefore.Valid { - t := lastBefore.Time - gap.LastFailureBeforeResolution = &t + for _, row := range lastRows { + if row.FailureTime.Valid { + t := row.FailureTime.Time + gap := gaps[row.RegressionID] + gap.LastFailureBeforeResolution = &t + gaps[row.RegressionID] = gap + } } - var firstAfter sql.NullTime - firstRow := prs.dbc.DB.Table("regression_job_runs"). - Where("regression_id = ? AND start_time > ? AND test_failed = true", regressionID, resolutionTime). - Select("MIN(start_time)").Row() - if err := firstRow.Scan(&firstAfter); err != nil { - return gap, fmt.Errorf("error querying first failure after resolution for regression %d: %w", regressionID, err) + // First failing run after the resolution time, one row per regression. + var firstRows []gapRow + if err := prs.dbc.DB.Table("regression_job_runs"). + Select("regression_id, MIN(start_time) AS failure_time"). + Where("regression_id IN ? AND start_time > ? AND test_failed = true", regressionIDs, resolutionTime). + Group("regression_id"). + Scan(&firstRows).Error; err != nil { + return nil, fmt.Errorf("error querying first failures after resolution: %w", err) } - if firstAfter.Valid { - t := firstAfter.Time - gap.FirstFailureAfterResolution = &t + for _, row := range firstRows { + if row.FailureTime.Valid { + t := row.FailureTime.Time + gap := gaps[row.RegressionID] + gap.FirstFailureAfterResolution = &t + gaps[row.RegressionID] = gap + } } - return gap, nil + return gaps, nil } // ForceClosePreview returns a dry-run of what ForceCloseRegressions would do for the given triage without @@ -414,7 +446,7 @@ func (prs *PostgresRegressionStore) queryRegressionFailureGap(regressionID uint, // includes the failure gap around the resolution time so callers can spot regressions that kept failing. func (prs *PostgresRegressionStore) ForceClosePreview(triageID uint) (*ForceClosePreview, error) { var triage models.Triage - if err := prs.dbc.DB.Preload("Regressions").First(&triage, triageID).Error; err != nil { + if err := prs.dbc.DB.First(&triage, triageID).Error; err != nil { return nil, fmt.Errorf("error loading triage %d for force close preview: %w", triageID, err) } if !triage.Resolved.Valid { @@ -423,18 +455,37 @@ func (prs *PostgresRegressionStore) ForceClosePreview(triageID uint) (*ForceClos resolved := triage.Resolved.Time preview := &ForceClosePreview{TriageID: triageID, Resolved: resolved} - for i := range triage.Regressions { - reg := &triage.Regressions[i] - gap, err := prs.queryRegressionFailureGap(reg.ID, resolved) - if err != nil { - return nil, err - } + // Load only the regressions the preview reports on: those still open (would close / would not close + // depending on when they opened) or those opened after the resolution time. Already-closed + // regressions that opened before resolution are omitted (force closing would not change them), so we + // filter them out in the query rather than loading every regression ever associated with the triage. + var regs []models.TestRegression + if err := prs.dbc.DB.Table(testRegressionsTable). + Select("test_regressions.*"). + Joins("JOIN triage_regressions ON triage_regressions.test_regression_id = test_regressions.id"). + Where("triage_regressions.triage_id = ?", triageID). + Where("test_regressions.closed IS NULL OR test_regressions.opened > ?", resolved). + Find(®s).Error; err != nil { + return nil, fmt.Errorf("error loading regressions for triage %d force close preview: %w", triageID, err) + } + + regIDs := make([]uint, len(regs)) + for i := range regs { + regIDs[i] = regs[i].ID + } + gaps, err := prs.queryRegressionFailureGaps(regIDs, resolved) + if err != nil { + return nil, err + } + + for i := range regs { + reg := ®s[i] entry := ForceClosePreviewRegression{ RegressionID: reg.ID, TestName: reg.TestName, Variants: reg.Variants, Opened: reg.Opened, - RegressionFailureGap: gap, + RegressionFailureGap: gaps[reg.ID], } if reg.Closed.Valid { c := reg.Closed.Time @@ -448,8 +499,6 @@ func (prs *PostgresRegressionStore) ForceClosePreview(triageID uint) (*ForceClos // Opened after the resolution time: force close would leave it untouched. preview.WouldNotClose = append(preview.WouldNotClose, entry) } - // Already-closed regressions that opened before resolution are omitted: force closing would not - // change them. } return preview, nil } diff --git a/pkg/db/migrations/000013_add_force_close_to_regressions.down.sql b/pkg/db/migrations/000013_add_force_close_to_regressions.down.sql index d893d62266..f2fab2a007 100644 --- a/pkg/db/migrations/000013_add_force_close_to_regressions.down.sql +++ b/pkg/db/migrations/000013_add_force_close_to_regressions.down.sql @@ -1,3 +1,4 @@ +DROP INDEX IF EXISTS idx_test_regressions_force_closed; DROP INDEX IF EXISTS idx_test_regressions_force_closed_by_triage_id; ALTER TABLE test_regressions diff --git a/pkg/db/migrations/000013_add_force_close_to_regressions.up.sql b/pkg/db/migrations/000013_add_force_close_to_regressions.up.sql index 59c83765c8..4bf2ea9abf 100644 --- a/pkg/db/migrations/000013_add_force_close_to_regressions.up.sql +++ b/pkg/db/migrations/000013_add_force_close_to_regressions.up.sql @@ -18,3 +18,8 @@ ALTER TABLE test_regressions CREATE INDEX IF NOT EXISTS idx_test_regressions_force_closed_by_triage_id ON test_regressions (force_closed_by_triage_id); + +-- Partial index so the reuse-window queries that exclude force closed regressions +-- (ListCurrentRegressionsForRelease, ResolveTriages) can skip the force closed rows +-- cheaply. Only the force closed rows are indexed. +CREATE INDEX idx_test_regressions_force_closed ON test_regressions (force_closed) WHERE force_closed = true; diff --git a/pkg/sippyserver/server.go b/pkg/sippyserver/server.go index 7665d4d246..dea261880d 100644 --- a/pkg/sippyserver/server.go +++ b/pkg/sippyserver/server.go @@ -1982,6 +1982,12 @@ func (s *Server) jsonForceCloseRegressions(w http.ResponseWriter, req *http.Requ } user := getUserForRequest(req) + // Force closing records who closed each regression for attribution and audit, so we refuse to + // proceed when we cannot determine the user rather than recording an empty ForceClosedBy. + if strings.TrimSpace(user) == "" { + failureResponse(w, http.StatusUnauthorized, "cannot determine user for force close request; authentication is required") + return + } log.Infof("triage force_close_regressions POST made by user: %s", user) var forceCloseReq forceCloseRegressionsRequest From a5053ff8cd21bb0fb5bd274af330f631721ec02a Mon Sep 17 00:00:00 2001 From: Chai Bot Date: Wed, 19 Aug 2026 13:52:34 +0000 Subject: [PATCH 03/10] TRT-2895: Apply force-close review fixes - Replace map[string]interface{} with map[string]any in ForceCloseRegressions. - Remove the transaction wrapper from ForceCloseRegressions: Postgres READ COMMITTED does not lock rows on SELECT, so the transaction provided no meaningful race protection; the single UPDATE ... WHERE id IN (...) is already atomic. - Expand the comment explaining why Updates() is used instead of Save() (Save() with an empty Triages slice would wipe the triage_regressions many2many join rows; Updates() with a column map never touches join tables). - Make force_closed_by and force_closed_reason nullable (*string in the model, plain nullable TEXT in the migration). NULL means "not applicable" (the regression was not force closed). - Drop the two unused indexes (idx_test_regressions_force_closed_by_triage_id and the partial idx_test_regressions_force_closed) from the up/down migrations, and drop the matching gorm index tag so AutoMigrate does not recreate the index. Co-Authored-By: Claude Opus 4.8 --- .../componentreadiness/regressiontracker.go | 79 ++++++++++--------- ...13_add_force_close_to_regressions.down.sql | 3 - ...0013_add_force_close_to_regressions.up.sql | 14 +--- pkg/db/models/triage.go | 12 +-- .../regressiontracker_test.go | 6 +- .../triage/triageapi_test.go | 12 ++- 6 files changed, 65 insertions(+), 61 deletions(-) diff --git a/pkg/api/componentreadiness/regressiontracker.go b/pkg/api/componentreadiness/regressiontracker.go index 7d4a097a78..bd90db580a 100644 --- a/pkg/api/componentreadiness/regressiontracker.go +++ b/pkg/api/componentreadiness/regressiontracker.go @@ -17,7 +17,6 @@ import ( "github.com/openshift/sippy/pkg/db" "github.com/openshift/sippy/pkg/db/models" log "github.com/sirupsen/logrus" - "gorm.io/gorm" "k8s.io/apimachinery/pkg/util/sets" ) @@ -333,51 +332,57 @@ type ForceClosePreview struct { func (prs *PostgresRegressionStore) ForceCloseRegressions(triageID uint, closedBy, reason string) (*ForceCloseResult, error) { result := &ForceCloseResult{} - err := prs.dbc.DB.Transaction(func(tx *gorm.DB) error { - var triage models.Triage - if err := tx.First(&triage, triageID).Error; err != nil { - return fmt.Errorf("error loading triage %d for force close: %w", triageID, err) - } - if !triage.Resolved.Valid { - return ErrTriageNotResolved - } - closeTime := triage.Resolved.Time - result.Timestamp = closeTime - - // Select only the regressions this triage actually resolved: those that existed at the - // resolution time (opened <= closeTime) and are still open (closed IS NULL). Filtering in the - // query rather than loading every regression ever associated with the triage keeps the action - // scoped, keeps it idempotent, and preserves the closed time of already-closed regressions. - var regIDsToClose []uint - if err := tx.Table(testRegressionsTable). - Joins("JOIN triage_regressions ON triage_regressions.test_regression_id = test_regressions.id"). - Where("triage_regressions.triage_id = ?", triageID). - Where("test_regressions.closed IS NULL"). - Where("test_regressions.opened <= ?", closeTime). - Pluck("test_regressions.id", ®IDsToClose).Error; err != nil { - return fmt.Errorf("error finding regressions to force close for triage %d: %w", triageID, err) - } - if len(regIDsToClose) == 0 { - return nil - } + // No transaction wrapper here: Postgres READ COMMITTED does not take row locks on SELECT, so + // wrapping the select-then-update below in a transaction would not prevent a concurrent writer + // from changing rows between the two statements. The single UPDATE ... WHERE id IN (...) is + // atomic on its own, which is all the consistency this operation needs. + var triage models.Triage + if err := prs.dbc.DB.First(&triage, triageID).Error; err != nil { + return nil, fmt.Errorf("error loading triage %d for force close: %w", triageID, err) + } + if !triage.Resolved.Valid { + return nil, ErrTriageNotResolved + } + closeTime := triage.Resolved.Time + result.Timestamp = closeTime + + // Select only the regressions this triage actually resolved: those that existed at the + // resolution time (opened <= closeTime) and are still open (closed IS NULL). Filtering in the + // query rather than loading every regression ever associated with the triage keeps the action + // scoped, keeps it idempotent, and preserves the closed time of already-closed regressions. + var regIDsToClose []uint + if err := prs.dbc.DB.Table(testRegressionsTable). + Joins("JOIN triage_regressions ON triage_regressions.test_regression_id = test_regressions.id"). + Where("triage_regressions.triage_id = ?", triageID). + Where("test_regressions.closed IS NULL"). + Where("test_regressions.opened <= ?", closeTime). + Pluck("test_regressions.id", ®IDsToClose).Error; err != nil { + return nil, fmt.Errorf("error finding regressions to force close for triage %d: %w", triageID, err) + } - // Update only the affected columns in a single statement to avoid rewriting the many2many - // triage associations. - updates := map[string]interface{}{ + if len(regIDsToClose) > 0 { + // Use Updates() with explicit column names instead of Save(). + // + // TestRegression has a many2many relationship with Triage via the + // triage_regressions join table (the Triages []Triage field with + // gorm:"many2many:triage_regressions"). GORM's Save() method + // processes all fields including associations — if the Triages + // slice is empty (because we didn't Preload it), Save() would + // delete all existing rows in triage_regressions for this + // regression, severing the triage links. Updates() with a column + // map only generates UPDATE SET for the named columns and never + // touches association join tables. + updates := map[string]any{ "closed": sql.NullTime{Valid: true, Time: closeTime}, "force_closed": true, "force_closed_by": closedBy, "force_closed_reason": reason, "force_closed_by_triage_id": triageID, } - if err := tx.Model(&models.TestRegression{}).Where("id IN ?", regIDsToClose).Updates(updates).Error; err != nil { - return fmt.Errorf("error force closing regressions for triage %d: %w", triageID, err) + if err := prs.dbc.DB.Model(&models.TestRegression{}).Where("id IN ?", regIDsToClose).Updates(updates).Error; err != nil { + return nil, fmt.Errorf("error force closing regressions for triage %d: %w", triageID, err) } result.ClosedRegressionIDs = regIDsToClose - return nil - }) - if err != nil { - return nil, err } log.WithField("triageID", triageID).WithField("closedBy", closedBy). diff --git a/pkg/db/migrations/000013_add_force_close_to_regressions.down.sql b/pkg/db/migrations/000013_add_force_close_to_regressions.down.sql index f2fab2a007..609bd53401 100644 --- a/pkg/db/migrations/000013_add_force_close_to_regressions.down.sql +++ b/pkg/db/migrations/000013_add_force_close_to_regressions.down.sql @@ -1,6 +1,3 @@ -DROP INDEX IF EXISTS idx_test_regressions_force_closed; -DROP INDEX IF EXISTS idx_test_regressions_force_closed_by_triage_id; - ALTER TABLE test_regressions DROP COLUMN IF EXISTS force_closed_by_triage_id, DROP COLUMN IF EXISTS force_closed_reason, diff --git a/pkg/db/migrations/000013_add_force_close_to_regressions.up.sql b/pkg/db/migrations/000013_add_force_close_to_regressions.up.sql index 4bf2ea9abf..8a1010d573 100644 --- a/pkg/db/migrations/000013_add_force_close_to_regressions.up.sql +++ b/pkg/db/migrations/000013_add_force_close_to_regressions.up.sql @@ -10,16 +10,10 @@ -- self-contained: it records that it was force closed, by whom, why, and which -- triage drove the action. Existing rows default to not force closed. +-- force_closed_by and force_closed_reason are nullable: NULL means "not applicable" (the regression +-- was not force closed), while a non-NULL value records who force closed it and why. ALTER TABLE test_regressions ADD COLUMN IF NOT EXISTS force_closed BOOLEAN NOT NULL DEFAULT false, - ADD COLUMN IF NOT EXISTS force_closed_by TEXT NOT NULL DEFAULT '', - ADD COLUMN IF NOT EXISTS force_closed_reason TEXT NOT NULL DEFAULT '', + ADD COLUMN IF NOT EXISTS force_closed_by TEXT, + ADD COLUMN IF NOT EXISTS force_closed_reason TEXT, ADD COLUMN IF NOT EXISTS force_closed_by_triage_id BIGINT; - -CREATE INDEX IF NOT EXISTS idx_test_regressions_force_closed_by_triage_id - ON test_regressions (force_closed_by_triage_id); - --- Partial index so the reuse-window queries that exclude force closed regressions --- (ListCurrentRegressionsForRelease, ResolveTriages) can skip the force closed rows --- cheaply. Only the force closed rows are indexed. -CREATE INDEX idx_test_regressions_force_closed ON test_regressions (force_closed) WHERE force_closed = true; diff --git a/pkg/db/models/triage.go b/pkg/db/models/triage.go index af7fd8e6f3..879812b283 100644 --- a/pkg/db/models/triage.go +++ b/pkg/db/models/triage.go @@ -235,14 +235,16 @@ type TestRegression struct { // record is self-contained and needs no join back to the triage. ForceClosed bool `json:"force_closed" gorm:"not null;default:false"` - // ForceClosedBy records the user who force closed this regression. - ForceClosedBy string `json:"force_closed_by" gorm:"default:''"` + // ForceClosedBy records the user who force closed this regression. It is nil when the regression + // was not force closed (NULL means not applicable). + ForceClosedBy *string `json:"force_closed_by"` - // ForceClosedReason records the user supplied reason for force closing this regression. - ForceClosedReason string `json:"force_closed_reason" gorm:"default:''"` + // ForceClosedReason records the user supplied reason for force closing this regression. It is nil + // when the regression was not force closed (NULL means not applicable). + ForceClosedReason *string `json:"force_closed_reason"` // ForceClosedByTriageID references the triage whose resolution drove the force close, if any. - ForceClosedByTriageID *uint `json:"force_closed_by_triage_id" gorm:"index"` + ForceClosedByTriageID *uint `json:"force_closed_by_triage_id"` // JobRuns accumulates the unique set of all job runs ever observed while this regression was open. // As the 7-day sample window slides, old runs roll off and new ones appear, but this list retains all of them. diff --git a/test/e2e/componentreadiness/regressiontracker/regressiontracker_test.go b/test/e2e/componentreadiness/regressiontracker/regressiontracker_test.go index d99601475d..85770f2f83 100644 --- a/test/e2e/componentreadiness/regressiontracker/regressiontracker_test.go +++ b/test/e2e/componentreadiness/regressiontracker/regressiontracker_test.go @@ -365,8 +365,10 @@ func Test_ForceCloseRegressions(t *testing.T) { assert.True(t, checkReg.Closed.Valid, "regression should be closed") assert.WithinDuration(t, resolved, checkReg.Closed.Time, time.Second, "regression should close at the resolution time") assert.True(t, checkReg.ForceClosed, "regression should be force closed") - assert.Equal(t, "developer", checkReg.ForceClosedBy, "regression should record who force closed it") - assert.Equal(t, "generic test, unrelated failures", checkReg.ForceClosedReason, "regression should record the reason") + require.NotNil(t, checkReg.ForceClosedBy, "regression should record who force closed it") + assert.Equal(t, "developer", *checkReg.ForceClosedBy, "regression should record who force closed it") + require.NotNil(t, checkReg.ForceClosedReason, "regression should record the reason") + assert.Equal(t, "generic test, unrelated failures", *checkReg.ForceClosedReason, "regression should record the reason") require.NotNil(t, checkReg.ForceClosedByTriageID) assert.Equal(t, triage.ID, *checkReg.ForceClosedByTriageID) }) diff --git a/test/e2e/componentreadiness/triage/triageapi_test.go b/test/e2e/componentreadiness/triage/triageapi_test.go index 28630f6a35..a27159328e 100644 --- a/test/e2e/componentreadiness/triage/triageapi_test.go +++ b/test/e2e/componentreadiness/triage/triageapi_test.go @@ -816,8 +816,10 @@ func Test_ForceCloseRegressionsAPI(t *testing.T) { var checkReg models.TestRegression require.NoError(t, dbc.DB.First(&checkReg, reg.ID).Error) assert.True(t, checkReg.ForceClosed) - assert.Equal(t, "developer", checkReg.ForceClosedBy) - assert.Equal(t, "generic test, unrelated failures", checkReg.ForceClosedReason) + require.NotNil(t, checkReg.ForceClosedBy) + assert.Equal(t, "developer", *checkReg.ForceClosedBy) + require.NotNil(t, checkReg.ForceClosedReason) + assert.Equal(t, "generic test, unrelated failures", *checkReg.ForceClosedReason) }) t.Run("regression detail exposes force close info", func(t *testing.T) { @@ -841,8 +843,10 @@ func Test_ForceCloseRegressionsAPI(t *testing.T) { assert.True(t, detail.ForceClosed, "regression detail should report force_closed") require.NotNil(t, detail.ForceClosedByTriageID) assert.Equal(t, triageResponse.ID, *detail.ForceClosedByTriageID) - assert.Equal(t, "developer", detail.ForceClosedBy, "detail should include force_closed_by directly from the regression") - assert.Equal(t, "detail exposure reason", detail.ForceClosedReason, "detail should include force_closed_reason directly from the regression") + require.NotNil(t, detail.ForceClosedBy, "detail should include force_closed_by directly from the regression") + assert.Equal(t, "developer", *detail.ForceClosedBy, "detail should include force_closed_by directly from the regression") + require.NotNil(t, detail.ForceClosedReason, "detail should include force_closed_reason directly from the regression") + assert.Equal(t, "detail exposure reason", *detail.ForceClosedReason, "detail should include force_closed_reason directly from the regression") }) t.Run("force close is idempotent over the API", func(t *testing.T) { From 0fded84324ff1db97abe94adfe6356d7fe8814a4 Mon Sep 17 00:00:00 2001 From: Chai Bot Date: Wed, 19 Aug 2026 15:01:27 +0000 Subject: [PATCH 04/10] TRT-2895: Address CodeRabbit review feedback - Stop logging authenticated user identity in force-close paths; log only triage ID and count of affected regressions. - Make ForceCloseRegressions atomic: a single UPDATE selects eligible regressions via a subquery on triage_regressions and returns closed IDs with RETURNING, removing the Pluck-then-Updates race. - Force-close handlers reject negative/non-numeric IDs (ParseUint) and return 404 when the triage is not found (gorm.ErrRecordNotFound). - README: document that reason is required and non-empty, rename the duplicate "### Response" heading to "### Preview response", and document HATEOAS links. - e2e tests: clean up triage associations before regressions and check GORM .Error on cleanup calls. - Wrap queryRegressionFailureGaps errors with direction and regression IDs. - Add FK on force_closed_by_triage_id -> triages(id) ON DELETE SET NULL (UP), with a matching DROP CONSTRAINT (DOWN). - Add HATEOAS links to ForceCloseResult, ForceClosePreview, and preview regression items (self/triage/regression detail). - Scope force close with strict inequality (opened < resolved) in both ForceCloseRegressions and ForceClosePreview; a regression opened at the exact resolution instant is not force closed. Add a boundary test. Co-Authored-By: Claude Opus 4.8 --- pkg/api/README.md | 23 ++- .../componentreadiness/regressiontracker.go | 138 ++++++++++++------ ...13_add_force_close_to_regressions.down.sql | 3 + ...0013_add_force_close_to_regressions.up.sql | 15 ++ pkg/sippyserver/server.go | 20 ++- .../regressiontracker_test.go | 48 +++++- .../triage/triageapi_test.go | 53 ++++--- 7 files changed, 223 insertions(+), 77 deletions(-) diff --git a/pkg/api/README.md b/pkg/api/README.md index 2e39ff6e63..1adcfb3517 100644 --- a/pkg/api/README.md +++ b/pkg/api/README.md @@ -648,15 +648,15 @@ Deletes a triage record. Endpoint: `POST /api/component_readiness/triages/{id}/force_close_regressions` Force closes the open regressions associated with a resolved triage that existed at its -resolution time (opened at or before `resolved`). Force closed regressions are excluded from +resolution time (opened strictly before `resolved`). Force closed regressions are excluded from the regression reuse window (regressionHysteresisDays), so they are not reopened for unrelated failures. This prevents generic tests (for example "install should succeed") from staying open for weeks with false "pants on fire" or "failed fix" status. Each regression is closed at the triage's resolution time and records, directly on the regression row, that it was force closed, by which user, for what reason, and the triage that -drove the action. The operation is idempotent: regressions that opened after the resolution -time, or that are already closed, are left untouched. +drove the action. The operation is idempotent: regressions that opened at or after the +resolution time, or that are already closed, are left untouched. The triage must be resolved. If it is not, the endpoint returns `400 Bad Request` with the message "Cannot force-close regressions for an unresolved triage. Resolve the triage first." @@ -668,12 +668,17 @@ This is a write endpoint and requires the `write_endpoints` capability. |--------|--------|----------------------------------------------------------|----------| | reason | String | The reason the regressions are being force closed. | Yes | +`reason` is required and must be non-empty (a blank or whitespace-only value returns +`400 Bad Request`). A non-numeric or negative triage `id` in the path returns `400 Bad Request`, +and a triage `id` that does not exist returns `404 Not Found`. + ### Response | Field | Type | Description | |-------------------------|-----------------|-----------------------------------------------------------------| | closed_regression_ids | Array of number | IDs of the regressions that were open and got closed. | | timestamp | String (time) | The closed time applied to the regressions (the resolution time). | +| links | Object | HATEOAS links (`self`, `triage`, `force_close`, `force_close_preview`). | The regression record returned by `GET /api/component_readiness/regressions/{id}` includes `force_closed`, `force_closed_by`, `force_closed_reason`, and `force_closed_by_triage_id` @@ -684,16 +689,19 @@ Endpoint: `GET /api/component_readiness/triages/{id}/force_close_preview` Previews (dry run) what `force_close_regressions` would do for a resolved triage, without modifying anything. Use it to review which regressions would close and to spot any that kept failing after the claimed resolution before committing. The triage must be resolved; otherwise -the endpoint returns `400 Bad Request` with the same message as the force close endpoint. +the endpoint returns `400 Bad Request` with the same message as the force close endpoint. A +non-numeric or negative triage `id` returns `400 Bad Request`, and a triage `id` that does not +exist returns `404 Not Found`. -### Response +### Preview response | Field | Type | Description | |-----------------|-----------------|---------------------------------------------------------------------| | triage_id | Number | The triage being previewed. | | resolved | String (time) | The triage's resolution time (the cutoff used for scoping). | -| would_close | Array of object | Open regressions that existed at the resolution time (would close). | -| would_not_close | Array of object | Regressions that opened after the resolution time (left untouched). | +| would_close | Array of object | Open regressions that opened strictly before the resolution time (would close). | +| would_not_close | Array of object | Regressions opened at or after the resolution time (left untouched). | +| links | Object | HATEOAS links (`self`, `triage`, `force_close`, `force_close_preview`). | Each regression object in `would_close` / `would_not_close` includes: @@ -706,3 +714,4 @@ Each regression object in `would_close` / `would_not_close` includes: | closed | String (time) | When the regression closed, if already closed. | | last_failure_before_resolution| String (time) | Most recent failing job run at or before the resolution time. | | first_failure_after_resolution| String (time) | Earliest failing job run after the resolution time, if any (a gap indicator that the test kept failing). | +| links | Object | HATEOAS links for the regression (`self` points to its detail endpoint). | diff --git a/pkg/api/componentreadiness/regressiontracker.go b/pkg/api/componentreadiness/regressiontracker.go index bd90db580a..5f4e99d4ee 100644 --- a/pkg/api/componentreadiness/regressiontracker.go +++ b/pkg/api/componentreadiness/regressiontracker.go @@ -17,6 +17,7 @@ import ( "github.com/openshift/sippy/pkg/db" "github.com/openshift/sippy/pkg/db/models" log "github.com/sirupsen/logrus" + "gorm.io/gorm/clause" "k8s.io/apimachinery/pkg/util/sets" ) @@ -288,6 +289,8 @@ type ForceCloseResult struct { ClosedRegressionIDs []uint `json:"closed_regression_ids"` // Timestamp is the closed time applied to the regressions (the triage's resolution time). Timestamp time.Time `json:"timestamp"` + // Links holds HATEOAS links for discoverability (for example self and the owning triage). + Links map[string]string `json:"links,omitempty"` } // RegressionFailureGap describes the failure timing around a triage's resolution for a single regression. @@ -311,20 +314,24 @@ type ForceClosePreviewRegression struct { Closed *time.Time `json:"closed,omitempty"` // RegressionFailureGap is embedded so its fields are promoted into this object's JSON. RegressionFailureGap + // Links holds HATEOAS links for this regression (for example self, the regression detail endpoint). + Links map[string]string `json:"links,omitempty"` } // ForceClosePreview is the dry-run result for force closing a triage's regressions. WouldClose lists the -// open regressions that existed at the resolution time and would be closed; WouldNotClose lists regressions -// that opened after the resolution time and would be left untouched. +// open regressions that opened strictly before the resolution time and would be closed; WouldNotClose lists +// regressions that opened at or after the resolution time and would be left untouched. type ForceClosePreview struct { TriageID uint `json:"triage_id"` Resolved time.Time `json:"resolved"` WouldClose []ForceClosePreviewRegression `json:"would_close"` WouldNotClose []ForceClosePreviewRegression `json:"would_not_close"` + // Links holds HATEOAS links for discoverability (for example self and the owning triage). + Links map[string]string `json:"links,omitempty"` } // ForceCloseRegressions closes the open regressions associated with the given resolved triage that existed -// at its resolution time (opened at or before triage.Resolved), marking them force closed so they are +// at its resolution time (opened strictly before triage.Resolved), marking them force closed so they are // excluded from the regression reuse window (regressionHysteresisDays) and never reopened for unrelated // failures (TRT-2895). Each regression is closed at the triage's resolution time and records who force // closed it and why, directly on the regression row. The triage must be resolved; otherwise @@ -332,10 +339,6 @@ type ForceClosePreview struct { func (prs *PostgresRegressionStore) ForceCloseRegressions(triageID uint, closedBy, reason string) (*ForceCloseResult, error) { result := &ForceCloseResult{} - // No transaction wrapper here: Postgres READ COMMITTED does not take row locks on SELECT, so - // wrapping the select-then-update below in a transaction would not prevent a concurrent writer - // from changing rows between the two statements. The single UPDATE ... WHERE id IN (...) is - // atomic on its own, which is all the consistency this operation needs. var triage models.Triage if err := prs.dbc.DB.First(&triage, triageID).Error; err != nil { return nil, fmt.Errorf("error loading triage %d for force close: %w", triageID, err) @@ -346,46 +349,43 @@ func (prs *PostgresRegressionStore) ForceCloseRegressions(triageID uint, closedB closeTime := triage.Resolved.Time result.Timestamp = closeTime - // Select only the regressions this triage actually resolved: those that existed at the - // resolution time (opened <= closeTime) and are still open (closed IS NULL). Filtering in the - // query rather than loading every regression ever associated with the triage keeps the action - // scoped, keeps it idempotent, and preserves the closed time of already-closed regressions. - var regIDsToClose []uint - if err := prs.dbc.DB.Table(testRegressionsTable). - Joins("JOIN triage_regressions ON triage_regressions.test_regression_id = test_regressions.id"). - Where("triage_regressions.triage_id = ?", triageID). + // Force close in a single atomic UPDATE. The eligible regressions are selected inside the + // statement via a subquery on the triage_regressions join table, so there is no window between + // finding the IDs and updating them in which a concurrent force close could see the same rows + // and overwrite each other's metadata. Eligibility: associated with this triage, still open + // (closed IS NULL), and opened strictly before the resolution time (opened < closeTime) so a + // regression opened at the exact resolution instant is left untouched. RETURNING id reports + // exactly which rows this call closed, keeping the operation idempotent (a repeat call matches + // nothing and returns an empty list). + // + // Updates() with a column map (not Save()) is deliberate: TestRegression has a many2many with + // Triage via triage_regressions, and Save() would rewrite that association join table. A column + // map only emits UPDATE SET for the named columns and never touches the join table. + var closed []models.TestRegression + res := prs.dbc.DB.Model(&closed). + Clauses(clause.Returning{Columns: []clause.Column{{Name: "id"}}}). + Where("test_regressions.id IN (?)", + prs.dbc.DB.Table("triage_regressions"). + Select("test_regression_id"). + Where("triage_id = ?", triageID)). Where("test_regressions.closed IS NULL"). - Where("test_regressions.opened <= ?", closeTime). - Pluck("test_regressions.id", ®IDsToClose).Error; err != nil { - return nil, fmt.Errorf("error finding regressions to force close for triage %d: %w", triageID, err) - } - - if len(regIDsToClose) > 0 { - // Use Updates() with explicit column names instead of Save(). - // - // TestRegression has a many2many relationship with Triage via the - // triage_regressions join table (the Triages []Triage field with - // gorm:"many2many:triage_regressions"). GORM's Save() method - // processes all fields including associations — if the Triages - // slice is empty (because we didn't Preload it), Save() would - // delete all existing rows in triage_regressions for this - // regression, severing the triage links. Updates() with a column - // map only generates UPDATE SET for the named columns and never - // touches association join tables. - updates := map[string]any{ + Where("test_regressions.opened < ?", closeTime). + Updates(map[string]any{ "closed": sql.NullTime{Valid: true, Time: closeTime}, "force_closed": true, "force_closed_by": closedBy, "force_closed_reason": reason, "force_closed_by_triage_id": triageID, - } - if err := prs.dbc.DB.Model(&models.TestRegression{}).Where("id IN ?", regIDsToClose).Updates(updates).Error; err != nil { - return nil, fmt.Errorf("error force closing regressions for triage %d: %w", triageID, err) - } - result.ClosedRegressionIDs = regIDsToClose + }) + if res.Error != nil { + return nil, fmt.Errorf("error force closing regressions for triage %d: %w", triageID, res.Error) + } + for i := range closed { + result.ClosedRegressionIDs = append(result.ClosedRegressionIDs, closed[i].ID) } - log.WithField("triageID", triageID).WithField("closedBy", closedBy). + // Do not log the user identity: log only the triage ID and how many regressions were closed. + log.WithField("triageID", triageID). WithField("closedRegressions", len(result.ClosedRegressionIDs)).Info("force closed regressions for triage") return result, nil } @@ -414,7 +414,7 @@ func (prs *PostgresRegressionStore) queryRegressionFailureGaps(regressionIDs []u Where("regression_id IN ? AND start_time <= ? AND test_failed = true", regressionIDs, resolutionTime). Group("regression_id"). Scan(&lastRows).Error; err != nil { - return nil, fmt.Errorf("error querying last failures before resolution: %w", err) + return nil, fmt.Errorf("error querying last failures before resolution for regressions %v: %w", regressionIDs, err) } for _, row := range lastRows { if row.FailureTime.Valid { @@ -432,7 +432,7 @@ func (prs *PostgresRegressionStore) queryRegressionFailureGaps(regressionIDs []u Where("regression_id IN ? AND start_time > ? AND test_failed = true", regressionIDs, resolutionTime). Group("regression_id"). Scan(&firstRows).Error; err != nil { - return nil, fmt.Errorf("error querying first failures after resolution: %w", err) + return nil, fmt.Errorf("error querying first failures after resolution for regressions %v: %w", regressionIDs, err) } for _, row := range firstRows { if row.FailureTime.Valid { @@ -461,7 +461,7 @@ func (prs *PostgresRegressionStore) ForceClosePreview(triageID uint) (*ForceClos preview := &ForceClosePreview{TriageID: triageID, Resolved: resolved} // Load only the regressions the preview reports on: those still open (would close / would not close - // depending on when they opened) or those opened after the resolution time. Already-closed + // depending on when they opened) or those opened at or after the resolution time. Already-closed // regressions that opened before resolution are omitted (force closing would not change them), so we // filter them out in the query rather than loading every regression ever associated with the triage. var regs []models.TestRegression @@ -469,7 +469,7 @@ func (prs *PostgresRegressionStore) ForceClosePreview(triageID uint) (*ForceClos Select("test_regressions.*"). Joins("JOIN triage_regressions ON triage_regressions.test_regression_id = test_regressions.id"). Where("triage_regressions.triage_id = ?", triageID). - Where("test_regressions.closed IS NULL OR test_regressions.opened > ?", resolved). + Where("test_regressions.closed IS NULL OR test_regressions.opened >= ?", resolved). Find(®s).Error; err != nil { return nil, fmt.Errorf("error loading regressions for triage %d force close preview: %w", triageID, err) } @@ -497,17 +497,61 @@ func (prs *PostgresRegressionStore) ForceClosePreview(triageID uint) (*ForceClos entry.Closed = &c } switch { - case !reg.Closed.Valid && !reg.Opened.After(resolved): - // Open and existed at the resolution time: this is what force close would close. + case !reg.Closed.Valid && reg.Opened.Before(resolved): + // Open and opened strictly before the resolution time: this is what force close would close. preview.WouldClose = append(preview.WouldClose, entry) - case reg.Opened.After(resolved): - // Opened after the resolution time: force close would leave it untouched. + case !reg.Opened.Before(resolved): + // Opened at or after the resolution time: force close would leave it untouched. A regression + // opened at the exact resolution instant is intentionally excluded from closing (strict <). preview.WouldNotClose = append(preview.WouldNotClose, entry) } } return preview, nil } +// forceCloseTriageLinks builds the HATEOAS links common to force close responses: the owning triage +// and the two force close endpoints for the triage. +func forceCloseTriageLinks(baseURL string, triageID uint) map[string]string { + return map[string]string{ + "triage": fmt.Sprintf("%s/api/component_readiness/triages/%d", baseURL, triageID), + "force_close": fmt.Sprintf("%s/api/component_readiness/triages/%d/force_close_regressions", baseURL, triageID), + "force_close_preview": fmt.Sprintf("%s/api/component_readiness/triages/%d/force_close_preview", baseURL, triageID), + } +} + +// regressionDetailLink builds the HATEOAS link to a single regression's detail endpoint. +func regressionDetailLink(baseURL string, regressionID uint) map[string]string { + return map[string]string{ + "self": fmt.Sprintf("%s/api/component_readiness/regressions/%d", baseURL, regressionID), + } +} + +// InjectForceCloseHATEOASLinks populates HATEOAS links on a force close result so clients can +// discover the owning triage and the related force close endpoints. +func InjectForceCloseHATEOASLinks(result *ForceCloseResult, baseURL string, triageID uint) { + if result == nil { + return + } + result.Links = forceCloseTriageLinks(baseURL, triageID) + result.Links["self"] = result.Links["force_close"] +} + +// InjectForceClosePreviewHATEOASLinks populates HATEOAS links on a force close preview and on each +// previewed regression (linking to its detail endpoint) for discoverability. +func InjectForceClosePreviewHATEOASLinks(preview *ForceClosePreview, baseURL string) { + if preview == nil { + return + } + preview.Links = forceCloseTriageLinks(baseURL, preview.TriageID) + preview.Links["self"] = preview.Links["force_close_preview"] + for i := range preview.WouldClose { + preview.WouldClose[i].Links = regressionDetailLink(baseURL, preview.WouldClose[i].RegressionID) + } + for i := range preview.WouldNotClose { + preview.WouldNotClose[i].Links = regressionDetailLink(baseURL, preview.WouldNotClose[i].RegressionID) + } +} + // SyncRegressionsForReport compares regressed tests from a component report against known // regressions in the database, opening new ones, reopening recently closed ones, and updating // stats on existing ones. Returns the list of active regressions after sync. diff --git a/pkg/db/migrations/000013_add_force_close_to_regressions.down.sql b/pkg/db/migrations/000013_add_force_close_to_regressions.down.sql index 609bd53401..42dc5de7ce 100644 --- a/pkg/db/migrations/000013_add_force_close_to_regressions.down.sql +++ b/pkg/db/migrations/000013_add_force_close_to_regressions.down.sql @@ -1,3 +1,6 @@ +ALTER TABLE test_regressions + DROP CONSTRAINT IF EXISTS fk_test_regressions_force_closed_by_triage; + ALTER TABLE test_regressions DROP COLUMN IF EXISTS force_closed_by_triage_id, DROP COLUMN IF EXISTS force_closed_reason, diff --git a/pkg/db/migrations/000013_add_force_close_to_regressions.up.sql b/pkg/db/migrations/000013_add_force_close_to_regressions.up.sql index 8a1010d573..18fc954ba5 100644 --- a/pkg/db/migrations/000013_add_force_close_to_regressions.up.sql +++ b/pkg/db/migrations/000013_add_force_close_to_regressions.up.sql @@ -17,3 +17,18 @@ ALTER TABLE test_regressions ADD COLUMN IF NOT EXISTS force_closed_by TEXT, ADD COLUMN IF NOT EXISTS force_closed_reason TEXT, ADD COLUMN IF NOT EXISTS force_closed_by_triage_id BIGINT; + +-- Tie force_closed_by_triage_id back to the driving triage. ON DELETE SET NULL keeps the historical +-- force close flag/by/reason on the regression while clearing the dangling reference if that triage +-- is later deleted. Guarded by a NOT EXISTS check because Postgres has no ADD CONSTRAINT IF NOT +-- EXISTS, keeping this migration idempotent alongside the ADD COLUMN IF NOT EXISTS above. +DO $$ +BEGIN + IF NOT EXISTS ( + SELECT 1 FROM pg_constraint WHERE conname = 'fk_test_regressions_force_closed_by_triage' + ) THEN + ALTER TABLE test_regressions + ADD CONSTRAINT fk_test_regressions_force_closed_by_triage + FOREIGN KEY (force_closed_by_triage_id) REFERENCES triages(id) ON DELETE SET NULL; + END IF; +END$$; diff --git a/pkg/sippyserver/server.go b/pkg/sippyserver/server.go index dea261880d..42d60a6e3b 100644 --- a/pkg/sippyserver/server.go +++ b/pkg/sippyserver/server.go @@ -1975,7 +1975,8 @@ type forceCloseRegressionsRequest struct { func (s *Server) jsonForceCloseRegressions(w http.ResponseWriter, req *http.Request) { vars := mux.Vars(req) idStr := vars["id"] - triageID, err := strconv.Atoi(idStr) + // ParseUint rejects negative and non-numeric IDs before we touch the database. + triageID, err := strconv.ParseUint(idStr, 10, 64) if err != nil { failureResponse(w, http.StatusBadRequest, "invalid ID format: "+idStr) return @@ -1983,12 +1984,12 @@ func (s *Server) jsonForceCloseRegressions(w http.ResponseWriter, req *http.Requ user := getUserForRequest(req) // Force closing records who closed each regression for attribution and audit, so we refuse to - // proceed when we cannot determine the user rather than recording an empty ForceClosedBy. + // proceed when we cannot determine the user rather than recording an empty ForceClosedBy. The + // user identity is intentionally not logged. if strings.TrimSpace(user) == "" { failureResponse(w, http.StatusUnauthorized, "cannot determine user for force close request; authentication is required") return } - log.Infof("triage force_close_regressions POST made by user: %s", user) var forceCloseReq forceCloseRegressionsRequest if err := json.NewDecoder(req.Body).Decode(&forceCloseReq); err != nil { @@ -2008,17 +2009,23 @@ func (s *Server) jsonForceCloseRegressions(w http.ResponseWriter, req *http.Requ failureResponse(w, http.StatusBadRequest, "Cannot force-close regressions for an unresolved triage. Resolve the triage first.") return } + if errors.Is(err, gorm.ErrRecordNotFound) { + failureResponse(w, http.StatusNotFound, fmt.Sprintf("triage %d not found", triageID)) + return + } log.WithError(err).Error("error force closing regressions") failureResponse(w, http.StatusInternalServerError, err.Error()) return } + componentreadiness.InjectForceCloseHATEOASLinks(result, api.GetBaseURL(req), uint(triageID)) // nolint:gosec api.RespondWithJSON(http.StatusOK, w, result) } func (s *Server) jsonForceClosePreview(w http.ResponseWriter, req *http.Request) { vars := mux.Vars(req) idStr := vars["id"] - triageID, err := strconv.Atoi(idStr) + // ParseUint rejects negative and non-numeric IDs before we touch the database. + triageID, err := strconv.ParseUint(idStr, 10, 64) if err != nil { failureResponse(w, http.StatusBadRequest, "invalid ID format: "+idStr) return @@ -2031,10 +2038,15 @@ func (s *Server) jsonForceClosePreview(w http.ResponseWriter, req *http.Request) failureResponse(w, http.StatusBadRequest, "Cannot force-close regressions for an unresolved triage. Resolve the triage first.") return } + if errors.Is(err, gorm.ErrRecordNotFound) { + failureResponse(w, http.StatusNotFound, fmt.Sprintf("triage %d not found", triageID)) + return + } log.WithError(err).Error("error building force close preview") failureResponse(w, http.StatusInternalServerError, err.Error()) return } + componentreadiness.InjectForceClosePreviewHATEOASLinks(preview, api.GetBaseURL(req)) api.RespondWithJSON(http.StatusOK, w, preview) } diff --git a/test/e2e/componentreadiness/regressiontracker/regressiontracker_test.go b/test/e2e/componentreadiness/regressiontracker/regressiontracker_test.go index 85770f2f83..00109a67d7 100644 --- a/test/e2e/componentreadiness/regressiontracker/regressiontracker_test.go +++ b/test/e2e/componentreadiness/regressiontracker/regressiontracker_test.go @@ -500,6 +500,35 @@ func Test_ForceCloseRegressions(t *testing.T) { assert.True(t, original.Closed.Valid, "force closed regression should remain closed") assert.True(t, original.ForceClosed, "force closed regression should remain force closed") }) + + t.Run("does not close a regression opened exactly at the resolution time", func(t *testing.T) { + defer cleanup() + + resolved := time.Now().Truncate(time.Second) + // Opened at the exact resolution instant: strict opened < resolved means this must not close. + reg, err := rawCreateRegression(dbc, "4.19", "fc-boundary", "force close test fc-boundary", + []string{"a:b"}, resolved, time.Time{}) + require.NoError(t, err) + triage := createResolvedTriageForRegressions(t, "https://redhat.atlassian.net/browse/TEST-FC-BOUNDARY", resolved, reg) + + result, err := tracker.ForceCloseRegressions(triage.ID, "developer", "boundary") + require.NoError(t, err) + assert.Empty(t, result.ClosedRegressionIDs, + "a regression opened exactly at the resolution time should not be force closed") + + var checkReg models.TestRegression + require.NoError(t, dbc.DB.First(&checkReg, reg.ID).Error) + assert.False(t, checkReg.Closed.Valid, "boundary regression should remain open") + assert.False(t, checkReg.ForceClosed, "boundary regression should not be force closed") + + // The preview must categorize the boundary regression under would_not_close, never would_close. + preview, err := tracker.ForceClosePreview(triage.ID) + require.NoError(t, err) + assert.True(t, containsPreviewRegression(preview.WouldNotClose, reg.ID), + "boundary regression should appear in would_not_close") + assert.False(t, containsPreviewRegression(preview.WouldClose, reg.ID), + "boundary regression should not appear in would_close") + }) } func containsRegression(regressions []*models.TestRegression, id uint) bool { @@ -511,6 +540,15 @@ func containsRegression(regressions []*models.TestRegression, id uint) bool { return false } +func containsPreviewRegression(regressions []componentreadiness.ForceClosePreviewRegression, id uint) bool { + for _, r := range regressions { + if r.RegressionID == id { + return true + } + } + return false +} + func cleanupJobRuns(dbc *db.DB) { res := dbc.DB.Where("1 = 1").Delete(&models.RegressionJobRun{}) if res.Error != nil { @@ -755,8 +793,14 @@ func Test_RegressionJobRuns(t *testing.T) { } func cleanupTriages(dbc *db.DB) { - dbc.DB.Exec("DELETE FROM triage_regressions WHERE 1=1") - dbc.DB.Where("1 = 1").Delete(&models.Triage{}) + // Delete the triage_regressions join rows before the triages so no association outlives a + // referenced row, and surface any error rather than silently leaking rows into later tests. + if err := dbc.DB.Exec("DELETE FROM triage_regressions WHERE 1=1").Error; err != nil { + log.Errorf("error deleting triage_regressions: %v", err) + } + if err := dbc.DB.Where("1 = 1").Delete(&models.Triage{}).Error; err != nil { + log.Errorf("error deleting triage records: %v", err) + } } func Test_SyncTriageSymptoms(t *testing.T) { diff --git a/test/e2e/componentreadiness/triage/triageapi_test.go b/test/e2e/componentreadiness/triage/triageapi_test.go index a27159328e..fb7831692c 100644 --- a/test/e2e/componentreadiness/triage/triageapi_test.go +++ b/test/e2e/componentreadiness/triage/triageapi_test.go @@ -41,8 +41,11 @@ var view = crview.View{ } func cleanupAllTriages(dbc *db.DB) { - // Delete all triage and test regressions in the e2e postgres db. - dbc.DB.Exec("DELETE FROM triage_regressions WHERE 1=1") + // Delete the triage_regressions join rows before the triages so no association outlives a + // referenced row. Errors are logged rather than swallowed so cleanup failures are visible. + if err := dbc.DB.Exec("DELETE FROM triage_regressions WHERE 1=1").Error; err != nil { + log.Errorf("error deleting triage_regressions: %v", err) + } res := dbc.DB.Where("1 = 1").Delete(&models.Triage{}) if res.Error != nil { log.Errorf("error deleting triage records: %v", res.Error) @@ -732,10 +735,34 @@ func Test_ForceCloseRegressionsAPI(t *testing.T) { tracker := componentreadiness.NewPostgresRegressionStore(dbc, nil) jiraBug := createBug(t, dbc.DB) - defer dbc.DB.Delete(jiraBug) + defer func() { + if err := dbc.DB.Delete(jiraBug).Error; err != nil { + log.Errorf("error deleting jira bug: %v", err) + } + }() release := view.SampleRelease.Name + // cleanupForceClose removes triage associations before the given regressions so the + // triage_regressions -> test_regressions FK ordering is respected (deleting a regression while a + // join row still references it would fail). It checks each delete so cleanup failures surface + // instead of silently leaking rows into later subtests. Defer it once per subtest; do not also + // defer a bare regression delete, which would run first (LIFO) and hit the FK. + cleanupForceClose := func(regs ...*models.TestRegression) { + cleanupAllTriages(dbc) + for _, reg := range regs { + if reg == nil { + continue + } + if err := dbc.DB.Where("test_regression_id = ?", reg.ID).Delete(&models.RegressionView{}).Error; err != nil { + log.Errorf("error deleting regression views for %d: %v", reg.ID, err) + } + if err := dbc.DB.Delete(reg).Error; err != nil { + log.Errorf("error deleting test regression %d: %v", reg.ID, err) + } + } + } + // createRawRegression creates a regression with a caller-controlled opened time so tests can exercise // the resolution-time scoping. createRawRegression := func(t *testing.T, testID string, opened time.Time) *models.TestRegression { @@ -760,9 +787,8 @@ func Test_ForceCloseRegressionsAPI(t *testing.T) { } t.Run("force close requires a reason", func(t *testing.T) { - defer cleanupAllTriages(dbc) reg := createTestRegression(t, tracker, view, "fc-api-reason") - defer dbc.DB.Delete(reg) + defer cleanupForceClose(reg) triageResponse := createAndValidateTriageRecord(t, jiraBug.URL, reg) resolveTriage(t, triageResponse, time.Now().Add(time.Minute)) @@ -774,9 +800,8 @@ func Test_ForceCloseRegressionsAPI(t *testing.T) { }) t.Run("force close on an unresolved triage is rejected", func(t *testing.T) { - defer cleanupAllTriages(dbc) reg := createTestRegression(t, tracker, view, "fc-api-unresolved") - defer dbc.DB.Delete(reg) + defer cleanupForceClose(reg) triageResponse := createAndValidateTriageRecord(t, jiraBug.URL, reg) // Force closing an unresolved triage must be rejected. @@ -792,9 +817,8 @@ func Test_ForceCloseRegressionsAPI(t *testing.T) { }) t.Run("force close closes regressions and excludes them from reuse", func(t *testing.T) { - defer cleanupAllTriages(dbc) reg := createTestRegression(t, tracker, view, "fc-api-close") - defer dbc.DB.Delete(reg) + defer cleanupForceClose(reg) triageResponse := createAndValidateTriageRecord(t, jiraBug.URL, reg) resolveTriage(t, triageResponse, time.Now().Add(time.Minute)) @@ -823,12 +847,10 @@ func Test_ForceCloseRegressionsAPI(t *testing.T) { }) t.Run("regression detail exposes force close info", func(t *testing.T) { - defer cleanupAllTriages(dbc) reg := createTestRegression(t, tracker, view, "fc-api-detail") - defer dbc.DB.Delete(reg) + defer cleanupForceClose(reg) // Associate with a view so the regression detail endpoint can build HATEOAS links. require.NoError(t, tracker.UpsertRegressionView(reg.ID, view.Name)) - defer dbc.DB.Where("test_regression_id = ?", reg.ID).Delete(&models.RegressionView{}) triageResponse := createAndValidateTriageRecord(t, jiraBug.URL, reg) resolveTriage(t, triageResponse, time.Now().Add(time.Minute)) @@ -850,9 +872,8 @@ func Test_ForceCloseRegressionsAPI(t *testing.T) { }) t.Run("force close is idempotent over the API", func(t *testing.T) { - defer cleanupAllTriages(dbc) reg := createTestRegression(t, tracker, view, "fc-api-idempotent") - defer dbc.DB.Delete(reg) + defer cleanupForceClose(reg) triageResponse := createAndValidateTriageRecord(t, jiraBug.URL, reg) resolveTriage(t, triageResponse, time.Now().Add(time.Minute)) @@ -870,13 +891,11 @@ func Test_ForceCloseRegressionsAPI(t *testing.T) { }) t.Run("preview lists would-close and would-not-close regressions with gap data", func(t *testing.T) { - defer cleanupAllTriages(dbc) resolved := time.Now().Add(-5 * 24 * time.Hour).Truncate(time.Second) wouldClose := createRawRegression(t, "fc-prev-close", resolved.Add(-10*24*time.Hour)) - defer dbc.DB.Delete(wouldClose) wouldNotClose := createRawRegression(t, "fc-prev-open", resolved.Add(24*time.Hour)) - defer dbc.DB.Delete(wouldNotClose) + defer cleanupForceClose(wouldClose, wouldNotClose) // Failures before and after the resolution time drive the gap indicator. require.NoError(t, tracker.MergeJobRuns(wouldClose.ID, []models.RegressionJobRun{ From 865b36c4a9c3bcfd292265bc14847cbb8ea186e4 Mon Sep 17 00:00:00 2001 From: Chai Bot Date: Wed, 19 Aug 2026 17:19:09 +0000 Subject: [PATCH 05/10] TRT-2895: Remove redundant force-close SQL migration GORM AutoMigrate already adds the force-close columns to test_regressions from the TestRegression struct tags, so the explicit SQL migration is redundant. Remove migration 000013 (up/down) and its MANIFEST entry. The force_closed_by_triage_id foreign key was informational only; the column is still created by AutoMigrate from the ForceClosedByTriageID *uint field, with no association or gorm constraint tag on the model. Co-Authored-By: Claude Opus 4.8 --- ...13_add_force_close_to_regressions.down.sql | 8 ----- ...0013_add_force_close_to_regressions.up.sql | 34 ------------------- pkg/db/migrations/MANIFEST | 1 - 3 files changed, 43 deletions(-) delete mode 100644 pkg/db/migrations/000013_add_force_close_to_regressions.down.sql delete mode 100644 pkg/db/migrations/000013_add_force_close_to_regressions.up.sql diff --git a/pkg/db/migrations/000013_add_force_close_to_regressions.down.sql b/pkg/db/migrations/000013_add_force_close_to_regressions.down.sql deleted file mode 100644 index 42dc5de7ce..0000000000 --- a/pkg/db/migrations/000013_add_force_close_to_regressions.down.sql +++ /dev/null @@ -1,8 +0,0 @@ -ALTER TABLE test_regressions - DROP CONSTRAINT IF EXISTS fk_test_regressions_force_closed_by_triage; - -ALTER TABLE test_regressions - DROP COLUMN IF EXISTS force_closed_by_triage_id, - DROP COLUMN IF EXISTS force_closed_reason, - DROP COLUMN IF EXISTS force_closed_by, - DROP COLUMN IF EXISTS force_closed; diff --git a/pkg/db/migrations/000013_add_force_close_to_regressions.up.sql b/pkg/db/migrations/000013_add_force_close_to_regressions.up.sql deleted file mode 100644 index 18fc954ba5..0000000000 --- a/pkg/db/migrations/000013_add_force_close_to_regressions.up.sql +++ /dev/null @@ -1,34 +0,0 @@ --- TRT-2895: Force close regressions. --- --- Generic tests (e.g. "install should succeed") stay open for weeks because the --- 5-day regression reuse window (regressionHysteresisDays) reopens recently --- closed regressions for unrelated failures, causing false "pants on fire" / --- "failed fix" status. Force closing a resolved triage's regressions marks them --- so they are excluded from the reuse window and never reopened. --- --- All force close metadata lives on test_regressions so a regression is --- self-contained: it records that it was force closed, by whom, why, and which --- triage drove the action. Existing rows default to not force closed. - --- force_closed_by and force_closed_reason are nullable: NULL means "not applicable" (the regression --- was not force closed), while a non-NULL value records who force closed it and why. -ALTER TABLE test_regressions - ADD COLUMN IF NOT EXISTS force_closed BOOLEAN NOT NULL DEFAULT false, - ADD COLUMN IF NOT EXISTS force_closed_by TEXT, - ADD COLUMN IF NOT EXISTS force_closed_reason TEXT, - ADD COLUMN IF NOT EXISTS force_closed_by_triage_id BIGINT; - --- Tie force_closed_by_triage_id back to the driving triage. ON DELETE SET NULL keeps the historical --- force close flag/by/reason on the regression while clearing the dangling reference if that triage --- is later deleted. Guarded by a NOT EXISTS check because Postgres has no ADD CONSTRAINT IF NOT --- EXISTS, keeping this migration idempotent alongside the ADD COLUMN IF NOT EXISTS above. -DO $$ -BEGIN - IF NOT EXISTS ( - SELECT 1 FROM pg_constraint WHERE conname = 'fk_test_regressions_force_closed_by_triage' - ) THEN - ALTER TABLE test_regressions - ADD CONSTRAINT fk_test_regressions_force_closed_by_triage - FOREIGN KEY (force_closed_by_triage_id) REFERENCES triages(id) ON DELETE SET NULL; - END IF; -END$$; diff --git a/pkg/db/migrations/MANIFEST b/pkg/db/migrations/MANIFEST index ae21f9d0bd..0c6d1c812e 100644 --- a/pkg/db/migrations/MANIFEST +++ b/pkg/db/migrations/MANIFEST @@ -19,4 +19,3 @@ 000010_drop_test_analysis_by_job_by_dates 000011_add_lifecycle_to_summaries 000012_drop_test_daily_totals_date_index -000013_add_force_close_to_regressions From 8f45360a367c01ddd3b87fa7988ca2b88e9771a0 Mon Sep 17 00:00:00 2001 From: Chai Bot Date: Wed, 19 Aug 2026 17:57:22 +0000 Subject: [PATCH 06/10] TRT-2895: Stop leaking internal errors from force-close API handlers The force-close and preview handlers passed err.Error() straight to the client, which can expose internal SQL/schema details. Log the detailed error server-side and return a generic message to the client instead. Co-Authored-By: Claude Opus 4.8 --- pkg/sippyserver/server.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/sippyserver/server.go b/pkg/sippyserver/server.go index 42d60a6e3b..cd66f7741a 100644 --- a/pkg/sippyserver/server.go +++ b/pkg/sippyserver/server.go @@ -1994,7 +1994,7 @@ func (s *Server) jsonForceCloseRegressions(w http.ResponseWriter, req *http.Requ var forceCloseReq forceCloseRegressionsRequest if err := json.NewDecoder(req.Body).Decode(&forceCloseReq); err != nil { log.WithError(err).Error("error parsing force close regressions request") - failureResponse(w, http.StatusBadRequest, err.Error()) + failureResponse(w, http.StatusBadRequest, "invalid request body") return } if strings.TrimSpace(forceCloseReq.Reason) == "" { @@ -2014,7 +2014,7 @@ func (s *Server) jsonForceCloseRegressions(w http.ResponseWriter, req *http.Requ return } log.WithError(err).Error("error force closing regressions") - failureResponse(w, http.StatusInternalServerError, err.Error()) + failureResponse(w, http.StatusInternalServerError, "failed to force close regressions") return } componentreadiness.InjectForceCloseHATEOASLinks(result, api.GetBaseURL(req), uint(triageID)) // nolint:gosec @@ -2043,7 +2043,7 @@ func (s *Server) jsonForceClosePreview(w http.ResponseWriter, req *http.Request) return } log.WithError(err).Error("error building force close preview") - failureResponse(w, http.StatusInternalServerError, err.Error()) + failureResponse(w, http.StatusInternalServerError, "failed to build force close preview") return } componentreadiness.InjectForceClosePreviewHATEOASLinks(preview, api.GetBaseURL(req)) From 4c268259957f77b77032a92fbf6e7130b0878943 Mon Sep 17 00:00:00 2001 From: Chai Bot Date: Wed, 19 Aug 2026 22:01:01 +0000 Subject: [PATCH 07/10] TRT-2895: Link regressions via association API in force-close e2e helper createTriageForRegressions embedded partial TestRegression objects (only the ID set) in the Triage passed to Create(). GORM treats the many2many Regressions field as an upsert and attempted to write those partial rows, violating the NOT NULL constraint on test_regressions.variants. Create the triage first without regressions, then link the existing regressions through the GORM association API (Association("Regressions"). Append) so no partial upsert occurs. Co-Authored-By: Claude Opus 4.8 --- .../regressiontracker/regressiontracker_test.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/test/e2e/componentreadiness/regressiontracker/regressiontracker_test.go b/test/e2e/componentreadiness/regressiontracker/regressiontracker_test.go index 00109a67d7..cb46489f58 100644 --- a/test/e2e/componentreadiness/regressiontracker/regressiontracker_test.go +++ b/test/e2e/componentreadiness/regressiontracker/regressiontracker_test.go @@ -317,18 +317,18 @@ func Test_ForceCloseRegressions(t *testing.T) { createTriageForRegressions := func(t *testing.T, url string, regs ...*models.TestRegression) models.Triage { t.Helper() - regressions := make([]models.TestRegression, len(regs)) - for i, r := range regs { - regressions[i] = models.TestRegression{ID: r.ID} - } triage := models.Triage{ URL: url, Description: "force close triage", Type: models.TriageTypeProduct, - Regressions: regressions, } dbWithContext := dbc.DB.WithContext(context.WithValue(context.Background(), models.CurrentUserKey, "e2e-test")) + // Create the triage first without regressions, then link the existing + // regressions via the association API. Embedding the regressions in the + // Create() call makes GORM upsert them, which writes the partial objects + // (nil Variants) and violates the NOT NULL constraint on variants. require.NoError(t, dbWithContext.Create(&triage).Error) + require.NoError(t, dbWithContext.Model(&triage).Association("Regressions").Append(regs)) return triage } From b02fcb4aba51ae9194eeed72520db709dacfed6e Mon Sep 17 00:00:00 2001 From: Chai Bot Date: Thu, 20 Aug 2026 00:41:18 +0000 Subject: [PATCH 08/10] TRT-2895: Add force-close integration tests and address review 5 Address CodeRabbit review 5 feedback and restructure the force-close test coverage: - server.go: reject trailing data after the force-close JSON body so {"reason":"a"}{"reason":"b"} is no longer silently accepted (the next decode must be io.EOF). - regressiontracker_test.go (e2e): correct the exclusive-boundary wording (opened < resolved is "before", not "at or before") by moving the DB-level time-scoping coverage to the integration suite, and make cleanupTriages return an error so deferred cleanups assert on it. - Add DB-level integration tests (test/integration/regression_forceclose_test.go) covering ForceCloseRegressions (basic, time-scoping, resolved guard, idempotency), ForceClosePreview (classification, gap data), ListCurrentRegressionsForRelease (force-closed exclusion), ResolveTriages (force-closed regressions do not block auto-resolution), and error paths. - Add reusable fixture helpers (CreateTestRegression, CreateTriage and functional options) to test/integration/util/fixtures.go, linking regressions via the Association API to avoid the GORM upsert footgun. - Slim the e2e force-close tests to the store/HTTP happy path plus the resolved/unresolved guard now that DB-level behavior is covered by the integration suite. - regressiontracker.go: enforce a non-empty force-close reason at the store layer (ErrForceCloseReasonRequired) so the invariant holds for every caller, matching the existing HTTP-layer check. Co-Authored-By: Claude Opus 4.8 --- .../componentreadiness/regressiontracker.go | 12 + pkg/sippyserver/server.go | 10 +- .../regressiontracker_test.go | 211 +----------- .../triage/triageapi_test.go | 138 +++----- .../integration/regression_forceclose_test.go | 317 ++++++++++++++++++ test/integration/util/fixtures.go | 100 ++++++ 6 files changed, 491 insertions(+), 297 deletions(-) create mode 100644 test/integration/regression_forceclose_test.go diff --git a/pkg/api/componentreadiness/regressiontracker.go b/pkg/api/componentreadiness/regressiontracker.go index 5f4e99d4ee..fe4e884b26 100644 --- a/pkg/api/componentreadiness/regressiontracker.go +++ b/pkg/api/componentreadiness/regressiontracker.go @@ -5,6 +5,7 @@ import ( "database/sql" "errors" "fmt" + "strings" "time" "github.com/andygrunwald/go-jira" @@ -281,6 +282,10 @@ func (prs *PostgresRegressionStore) ResolveTriages() error { // Force closing needs a resolution time to scope which regressions to close and when to close them. var ErrTriageNotResolved = errors.New("triage is not resolved") +// ErrForceCloseReasonRequired is returned by ForceCloseRegressions when called without a reason. Force +// close records the reason on each regression for audit, so a reason is required regardless of caller. +var ErrForceCloseReasonRequired = errors.New("a reason is required to force close regressions") + // ForceCloseResult summarizes the outcome of a force close operation. It is returned by the API // so callers know which regressions were closed and at what time. type ForceCloseResult struct { @@ -339,6 +344,13 @@ type ForceClosePreview struct { func (prs *PostgresRegressionStore) ForceCloseRegressions(triageID uint, closedBy, reason string) (*ForceCloseResult, error) { result := &ForceCloseResult{} + // A reason is recorded on each force closed regression for audit, so refuse to proceed without one + // regardless of caller. The HTTP handler validates this earlier to return a precise status; this + // store-level check keeps the invariant for any caller. + if strings.TrimSpace(reason) == "" { + return nil, ErrForceCloseReasonRequired + } + var triage models.Triage if err := prs.dbc.DB.First(&triage, triageID).Error; err != nil { return nil, fmt.Errorf("error loading triage %d for force close: %w", triageID, err) diff --git a/pkg/sippyserver/server.go b/pkg/sippyserver/server.go index cd66f7741a..d9db029e5f 100644 --- a/pkg/sippyserver/server.go +++ b/pkg/sippyserver/server.go @@ -1992,11 +1992,19 @@ func (s *Server) jsonForceCloseRegressions(w http.ResponseWriter, req *http.Requ } var forceCloseReq forceCloseRegressionsRequest - if err := json.NewDecoder(req.Body).Decode(&forceCloseReq); err != nil { + decoder := json.NewDecoder(req.Body) + if err := decoder.Decode(&forceCloseReq); err != nil { log.WithError(err).Error("error parsing force close regressions request") failureResponse(w, http.StatusBadRequest, "invalid request body") return } + // Reject any trailing data after the JSON object so bodies like {"reason":"first"}{"reason":"second"} + // are not silently accepted with only the first object honored. A well formed body decodes to exactly + // one value, after which the next decode must report io.EOF. + if err := decoder.Decode(&struct{}{}); err != io.EOF { + failureResponse(w, http.StatusBadRequest, "request body must contain a single JSON object") + return + } if strings.TrimSpace(forceCloseReq.Reason) == "" { failureResponse(w, http.StatusBadRequest, "reason is required to force close regressions") return diff --git a/test/e2e/componentreadiness/regressiontracker/regressiontracker_test.go b/test/e2e/componentreadiness/regressiontracker/regressiontracker_test.go index cb46489f58..bdb9838021 100644 --- a/test/e2e/componentreadiness/regressiontracker/regressiontracker_test.go +++ b/test/e2e/componentreadiness/regressiontracker/regressiontracker_test.go @@ -3,6 +3,7 @@ package regressiontracker import ( "context" "database/sql" + "fmt" "testing" "time" @@ -276,44 +277,6 @@ func Test_RegressionTracker(t *testing.T) { func Test_ForceCloseRegressions(t *testing.T) { dbc := util.CreateE2EPostgresConnection(t) tracker := componentreadiness.NewPostgresRegressionStore(dbc, nil) - rLog := log.WithField("test", "force-close-regressions") - - forceView := crview.View{ - Name: "4.19-main", - SampleRelease: reqopts.RelativeRelease{ - Release: reqopts.Release{Name: "4.19"}, - }, - BaseRelease: reqopts.RelativeRelease{ - Release: reqopts.Release{Name: "4.18"}, - }, - } - - newRegSummary := func(testID string) componentreport.ReportTestSummary { - return componentreport.ReportTestSummary{ - TestComparison: testdetails.TestComparison{ - BaseStats: &testdetails.ReleaseStats{Release: "4.18"}, - }, - Identification: crtest.Identification{ - RowIdentification: crtest.RowIdentification{ - Component: "comp", - Capability: "cap", - TestName: "force close test " + testID, - TestID: testID, - }, - ColumnIdentification: crtest.ColumnIdentification{ - Variants: map[string]string{"a": "b"}, - }, - }, - } - } - - makeReport := func(tests ...componentreport.ReportTestSummary) *componentreport.ComponentReport { - return &componentreport.ComponentReport{ - Rows: []componentreport.ReportRow{ - {Columns: []componentreport.ReportColumn{{RegressedTests: tests}}}, - }, - } - } createTriageForRegressions := func(t *testing.T, url string, regs ...*models.TestRegression) models.Triage { t.Helper() @@ -341,7 +304,7 @@ func Test_ForceCloseRegressions(t *testing.T) { } cleanup := func() { - cleanupTriages(dbc) + assert.NoError(t, cleanupTriages(dbc)) cleanupAllRegressions(dbc) } @@ -391,162 +354,6 @@ func Test_ForceCloseRegressions(t *testing.T) { assert.False(t, checkReg.Closed.Valid, "regression should remain open") assert.False(t, checkReg.ForceClosed, "regression should not be force closed") }) - - t.Run("only closes regressions that existed at the resolution time", func(t *testing.T) { - defer cleanup() - - resolved := time.Now().Add(-5 * 24 * time.Hour).Truncate(time.Second) - before, err := rawCreateRegression(dbc, "4.19", "fc-before", "force close test fc-before", - []string{"a:b"}, resolved.Add(-5*24*time.Hour), time.Time{}) - require.NoError(t, err) - after, err := rawCreateRegression(dbc, "4.19", "fc-after", "force close test fc-after", - []string{"a:b"}, resolved.Add(24*time.Hour), time.Time{}) - require.NoError(t, err) - triage := createResolvedTriageForRegressions(t, "https://redhat.atlassian.net/browse/TEST-FC-SCOPE", resolved, before, after) - - result, err := tracker.ForceCloseRegressions(triage.ID, "developer", "scoped close") - require.NoError(t, err) - assert.ElementsMatch(t, []uint{before.ID}, result.ClosedRegressionIDs, - "only the regression opened at or before the resolution time should be closed") - - var checkBefore models.TestRegression - require.NoError(t, dbc.DB.First(&checkBefore, before.ID).Error) - assert.True(t, checkBefore.ForceClosed, "regression opened before resolution should be force closed") - assert.WithinDuration(t, resolved, checkBefore.Closed.Time, time.Second) - - var checkAfter models.TestRegression - require.NoError(t, dbc.DB.First(&checkAfter, after.ID).Error) - assert.False(t, checkAfter.Closed.Valid, "regression opened after resolution should remain open") - assert.False(t, checkAfter.ForceClosed, "regression opened after resolution should not be force closed") - }) - - t.Run("is idempotent", func(t *testing.T) { - defer cleanup() - - resolved := time.Now().Truncate(time.Second) - reg, err := rawCreateRegression(dbc, "4.19", "fc-idempotent", "force close test fc-idempotent", - []string{"a:b"}, resolved.Add(-10*24*time.Hour), time.Time{}) - require.NoError(t, err) - triage := createResolvedTriageForRegressions(t, "https://redhat.atlassian.net/browse/TEST-FC-3", resolved, reg) - - result1, err := tracker.ForceCloseRegressions(triage.ID, "developer", "first call") - require.NoError(t, err) - require.Len(t, result1.ClosedRegressionIDs, 1) - - var closedReg models.TestRegression - require.NoError(t, dbc.DB.First(&closedReg, reg.ID).Error) - originalClosedTime := closedReg.Closed.Time - - // Second call should be a no-op: no open regressions remain. - result2, err := tracker.ForceCloseRegressions(triage.ID, "developer", "first call") - require.NoError(t, err) - assert.Empty(t, result2.ClosedRegressionIDs, "no regressions should be closed on repeat call") - - require.NoError(t, dbc.DB.First(&closedReg, reg.ID).Error) - assert.WithinDuration(t, originalClosedTime, closedReg.Closed.Time, time.Second, - "closed time should not change on repeat call") - }) - - t.Run("force closed regression excluded from ListCurrentRegressionsForRelease", func(t *testing.T) { - defer cleanup() - - resolved := time.Now().Truncate(time.Second) - reg, err := rawCreateRegression(dbc, "4.19", "fc-excluded", "force close test fc-excluded", - []string{"a:b"}, resolved.Add(-24*time.Hour), time.Time{}) - require.NoError(t, err) - triage := createResolvedTriageForRegressions(t, "https://redhat.atlassian.net/browse/TEST-FC-4", resolved, reg) - - // Before force close, the recently opened regression is listed. - before, err := tracker.ListCurrentRegressionsForRelease("4.19") - require.NoError(t, err) - assert.True(t, containsRegression(before, reg.ID), "open regression should be listed before force close") - - _, err = tracker.ForceCloseRegressions(triage.ID, "developer", "exclude from reuse") - require.NoError(t, err) - - // After force close, even though it closed just now (within the hysteresis window), it must be excluded. - after, err := tracker.ListCurrentRegressionsForRelease("4.19") - require.NoError(t, err) - assert.False(t, containsRegression(after, reg.ID), "force closed regression should be excluded from reuse list") - }) - - t.Run("force closed regression is not reused by SyncRegressionsForReport", func(t *testing.T) { - defer cleanup() - - report := makeReport(newRegSummary("fc-not-reused")) - - // First sync opens a regression. - regs1, err := componentreadiness.SyncRegressionsForReport(tracker, forceView, rLog, report) - require.NoError(t, err) - require.Len(t, regs1, 1) - originalID := regs1[0].ID - - // Force close it via a resolved triage. - triage := createResolvedTriageForRegressions(t, "https://redhat.atlassian.net/browse/TEST-FC-5", - time.Now().Add(time.Minute), regs1[0]) - _, err = tracker.ForceCloseRegressions(triage.ID, "developer", "not reused") - require.NoError(t, err) - - // Re-sync the same report: a brand new regression should open, not the force closed one. - regs2, err := componentreadiness.SyncRegressionsForReport(tracker, forceView, rLog, report) - require.NoError(t, err) - require.Len(t, regs2, 1) - assert.NotEqual(t, originalID, regs2[0].ID, - "a new regression should open instead of reusing the force closed one") - - // The original stays closed and force closed. - var original models.TestRegression - require.NoError(t, dbc.DB.First(&original, originalID).Error) - assert.True(t, original.Closed.Valid, "force closed regression should remain closed") - assert.True(t, original.ForceClosed, "force closed regression should remain force closed") - }) - - t.Run("does not close a regression opened exactly at the resolution time", func(t *testing.T) { - defer cleanup() - - resolved := time.Now().Truncate(time.Second) - // Opened at the exact resolution instant: strict opened < resolved means this must not close. - reg, err := rawCreateRegression(dbc, "4.19", "fc-boundary", "force close test fc-boundary", - []string{"a:b"}, resolved, time.Time{}) - require.NoError(t, err) - triage := createResolvedTriageForRegressions(t, "https://redhat.atlassian.net/browse/TEST-FC-BOUNDARY", resolved, reg) - - result, err := tracker.ForceCloseRegressions(triage.ID, "developer", "boundary") - require.NoError(t, err) - assert.Empty(t, result.ClosedRegressionIDs, - "a regression opened exactly at the resolution time should not be force closed") - - var checkReg models.TestRegression - require.NoError(t, dbc.DB.First(&checkReg, reg.ID).Error) - assert.False(t, checkReg.Closed.Valid, "boundary regression should remain open") - assert.False(t, checkReg.ForceClosed, "boundary regression should not be force closed") - - // The preview must categorize the boundary regression under would_not_close, never would_close. - preview, err := tracker.ForceClosePreview(triage.ID) - require.NoError(t, err) - assert.True(t, containsPreviewRegression(preview.WouldNotClose, reg.ID), - "boundary regression should appear in would_not_close") - assert.False(t, containsPreviewRegression(preview.WouldClose, reg.ID), - "boundary regression should not appear in would_close") - }) -} - -func containsRegression(regressions []*models.TestRegression, id uint) bool { - for _, r := range regressions { - if r.ID == id { - return true - } - } - return false -} - -func containsPreviewRegression(regressions []componentreadiness.ForceClosePreviewRegression, id uint) bool { - for _, r := range regressions { - if r.RegressionID == id { - return true - } - } - return false } func cleanupJobRuns(dbc *db.DB) { @@ -792,15 +599,17 @@ func Test_RegressionJobRuns(t *testing.T) { }) } -func cleanupTriages(dbc *db.DB) { - // Delete the triage_regressions join rows before the triages so no association outlives a - // referenced row, and surface any error rather than silently leaking rows into later tests. +// cleanupTriages removes triage_regressions join rows before the triages so no association outlives a +// referenced row. It returns an error rather than only logging so a deferred cleanup can assert on it and +// surface leaks instead of silently letting rows bleed into later tests. +func cleanupTriages(dbc *db.DB) error { if err := dbc.DB.Exec("DELETE FROM triage_regressions WHERE 1=1").Error; err != nil { - log.Errorf("error deleting triage_regressions: %v", err) + return fmt.Errorf("error deleting triage_regressions: %w", err) } if err := dbc.DB.Where("1 = 1").Delete(&models.Triage{}).Error; err != nil { - log.Errorf("error deleting triage records: %v", err) + return fmt.Errorf("error deleting triage records: %w", err) } + return nil } func Test_SyncTriageSymptoms(t *testing.T) { @@ -839,7 +648,7 @@ func Test_SyncTriageSymptoms(t *testing.T) { cleanup := func() { util.CleanupTriageSymptoms(dbc) cleanupJobRuns(dbc) - cleanupTriages(dbc) + assert.NoError(t, cleanupTriages(dbc)) cleanupAllRegressions(dbc) } diff --git a/test/e2e/componentreadiness/triage/triageapi_test.go b/test/e2e/componentreadiness/triage/triageapi_test.go index fb7831692c..615f9ef077 100644 --- a/test/e2e/componentreadiness/triage/triageapi_test.go +++ b/test/e2e/componentreadiness/triage/triageapi_test.go @@ -786,19 +786,6 @@ func Test_ForceCloseRegressionsAPI(t *testing.T) { require.NoError(t, util.SippyPut(fmt.Sprintf("/api/component_readiness/triages/%d", triageResp.ID), &triageResp, &updated)) } - t.Run("force close requires a reason", func(t *testing.T) { - reg := createTestRegression(t, tracker, view, "fc-api-reason") - defer cleanupForceClose(reg) - triageResponse := createAndValidateTriageRecord(t, jiraBug.URL, reg) - resolveTriage(t, triageResponse, time.Now().Add(time.Minute)) - - // Empty reason should be rejected. - var result componentreadiness.ForceCloseResult - err := util.SippyPost(fmt.Sprintf("/api/component_readiness/triages/%d/force_close_regressions", triageResponse.ID), - &map[string]string{"reason": ""}, &result) - require.Error(t, err, "force close with empty reason should fail") - }) - t.Run("force close on an unresolved triage is rejected", func(t *testing.T) { reg := createTestRegression(t, tracker, view, "fc-api-unresolved") defer cleanupForceClose(reg) @@ -816,93 +803,24 @@ func Test_ForceCloseRegressionsAPI(t *testing.T) { require.Error(t, err, "previewing an unresolved triage should fail") }) - t.Run("force close closes regressions and excludes them from reuse", func(t *testing.T) { - reg := createTestRegression(t, tracker, view, "fc-api-close") - defer cleanupForceClose(reg) - triageResponse := createAndValidateTriageRecord(t, jiraBug.URL, reg) - resolveTriage(t, triageResponse, time.Now().Add(time.Minute)) - - var result componentreadiness.ForceCloseResult - err := util.SippyPost(fmt.Sprintf("/api/component_readiness/triages/%d/force_close_regressions", triageResponse.ID), - &map[string]string{"reason": "generic test, unrelated failures"}, &result) - require.NoError(t, err) - assert.ElementsMatch(t, []uint{reg.ID}, result.ClosedRegressionIDs) - assert.False(t, result.Timestamp.IsZero()) - - // Regression should be closed and excluded from the reuse list. - regressions, err := tracker.ListCurrentRegressionsForRelease(release) - require.NoError(t, err) - for _, r := range regressions { - assert.NotEqual(t, reg.ID, r.ID, "force closed regression should not appear in reuse list") - } - - // The regression records who force closed it and why, directly on the regression row. - var checkReg models.TestRegression - require.NoError(t, dbc.DB.First(&checkReg, reg.ID).Error) - assert.True(t, checkReg.ForceClosed) - require.NotNil(t, checkReg.ForceClosedBy) - assert.Equal(t, "developer", *checkReg.ForceClosedBy) - require.NotNil(t, checkReg.ForceClosedReason) - assert.Equal(t, "generic test, unrelated failures", *checkReg.ForceClosedReason) - }) - - t.Run("regression detail exposes force close info", func(t *testing.T) { - reg := createTestRegression(t, tracker, view, "fc-api-detail") - defer cleanupForceClose(reg) - // Associate with a view so the regression detail endpoint can build HATEOAS links. - require.NoError(t, tracker.UpsertRegressionView(reg.ID, view.Name)) - triageResponse := createAndValidateTriageRecord(t, jiraBug.URL, reg) - resolveTriage(t, triageResponse, time.Now().Add(time.Minute)) - - var result componentreadiness.ForceCloseResult - err := util.SippyPost(fmt.Sprintf("/api/component_readiness/triages/%d/force_close_regressions", triageResponse.ID), - &map[string]string{"reason": "detail exposure reason"}, &result) - require.NoError(t, err) - - var detail models.TestRegression - err = util.SippyGet(fmt.Sprintf("/api/component_readiness/regressions/%d", reg.ID), &detail) - require.NoError(t, err) - assert.True(t, detail.ForceClosed, "regression detail should report force_closed") - require.NotNil(t, detail.ForceClosedByTriageID) - assert.Equal(t, triageResponse.ID, *detail.ForceClosedByTriageID) - require.NotNil(t, detail.ForceClosedBy, "detail should include force_closed_by directly from the regression") - assert.Equal(t, "developer", *detail.ForceClosedBy, "detail should include force_closed_by directly from the regression") - require.NotNil(t, detail.ForceClosedReason, "detail should include force_closed_reason directly from the regression") - assert.Equal(t, "detail exposure reason", *detail.ForceClosedReason, "detail should include force_closed_reason directly from the regression") - }) - - t.Run("force close is idempotent over the API", func(t *testing.T) { - reg := createTestRegression(t, tracker, view, "fc-api-idempotent") - defer cleanupForceClose(reg) - triageResponse := createAndValidateTriageRecord(t, jiraBug.URL, reg) - resolveTriage(t, triageResponse, time.Now().Add(time.Minute)) - - var result1 componentreadiness.ForceCloseResult - err := util.SippyPost(fmt.Sprintf("/api/component_readiness/triages/%d/force_close_regressions", triageResponse.ID), - &map[string]string{"reason": "idempotent"}, &result1) - require.NoError(t, err) - require.Len(t, result1.ClosedRegressionIDs, 1) - - var result2 componentreadiness.ForceCloseResult - err = util.SippyPost(fmt.Sprintf("/api/component_readiness/triages/%d/force_close_regressions", triageResponse.ID), - &map[string]string{"reason": "idempotent"}, &result2) - require.NoError(t, err) - assert.Empty(t, result2.ClosedRegressionIDs, "repeat force close should close no additional regressions") - }) - - t.Run("preview lists would-close and would-not-close regressions with gap data", func(t *testing.T) { + t.Run("create, resolve, preview, force close, and verify over the API", func(t *testing.T) { resolved := time.Now().Add(-5 * 24 * time.Hour).Truncate(time.Second) - wouldClose := createRawRegression(t, "fc-prev-close", resolved.Add(-10*24*time.Hour)) - wouldNotClose := createRawRegression(t, "fc-prev-open", resolved.Add(24*time.Hour)) + // One regression opened before the resolution time (force close should close it) and one opened + // after (force close should leave it open), so the happy path exercises resolution-time scoping. + wouldClose := createRawRegression(t, "fc-api-close", resolved.Add(-10*24*time.Hour)) + wouldNotClose := createRawRegression(t, "fc-api-open", resolved.Add(24*time.Hour)) defer cleanupForceClose(wouldClose, wouldNotClose) + // Associate the closing regression with a view so the regression detail endpoint can build links. + require.NoError(t, tracker.UpsertRegressionView(wouldClose.ID, view.Name)) - // Failures before and after the resolution time drive the gap indicator. + // Failures before and after the resolution time drive the preview gap indicator. require.NoError(t, tracker.MergeJobRuns(wouldClose.ID, []models.RegressionJobRun{ - {ProwJobRunID: "prev-before", ProwJobName: "job-1", StartTime: resolved.Add(-2 * 24 * time.Hour), TestFailed: true}, - {ProwJobRunID: "prev-after", ProwJobName: "job-1", StartTime: resolved.Add(2 * 24 * time.Hour), TestFailed: true}, + {ProwJobRunID: "api-before", ProwJobName: "job-1", StartTime: resolved.Add(-2 * 24 * time.Hour), TestFailed: true}, + {ProwJobRunID: "api-after", ProwJobName: "job-1", StartTime: resolved.Add(2 * 24 * time.Hour), TestFailed: true}, })) + // Create the triage over both regressions, then resolve it via the API. triage := models.Triage{ URL: jiraBug.URL, Type: models.TriageTypeProduct, @@ -915,18 +833,48 @@ func Test_ForceCloseRegressionsAPI(t *testing.T) { require.NoError(t, util.SippyPost("/api/component_readiness/triages", &triage, &triageResp)) resolveTriage(t, triageResp, resolved) + // Preview classifies the regressions and reports the failure gap around the resolution time. var preview componentreadiness.ForceClosePreview require.NoError(t, util.SippyGet(fmt.Sprintf("/api/component_readiness/triages/%d/force_close_preview", triageResp.ID), &preview)) - require.Len(t, preview.WouldClose, 1, "regression opened before resolution should be in would_close") assert.Equal(t, wouldClose.ID, preview.WouldClose[0].RegressionID) require.NotNil(t, preview.WouldClose[0].LastFailureBeforeResolution, "should report last failure before resolution") assert.WithinDuration(t, resolved.Add(-2*24*time.Hour), *preview.WouldClose[0].LastFailureBeforeResolution, time.Second) require.NotNil(t, preview.WouldClose[0].FirstFailureAfterResolution, "should report first failure after resolution") assert.WithinDuration(t, resolved.Add(2*24*time.Hour), *preview.WouldClose[0].FirstFailureAfterResolution, time.Second) - require.Len(t, preview.WouldNotClose, 1, "regression opened after resolution should be in would_not_close") assert.Equal(t, wouldNotClose.ID, preview.WouldNotClose[0].RegressionID) + + // Force close over the API and confirm exactly the eligible regression closed. + var result componentreadiness.ForceCloseResult + require.NoError(t, util.SippyPost(fmt.Sprintf("/api/component_readiness/triages/%d/force_close_regressions", triageResp.ID), + &map[string]string{"reason": "generic test, unrelated failures"}, &result)) + assert.ElementsMatch(t, []uint{wouldClose.ID}, result.ClosedRegressionIDs) + assert.False(t, result.Timestamp.IsZero()) + + // The force closed regression is excluded from the reuse list. + regressions, err := tracker.ListCurrentRegressionsForRelease(release) + require.NoError(t, err) + for _, r := range regressions { + assert.NotEqual(t, wouldClose.ID, r.ID, "force closed regression should not appear in reuse list") + } + + // Force close metadata is recorded on the regression row and surfaced by the detail endpoint. + var detail models.TestRegression + require.NoError(t, util.SippyGet(fmt.Sprintf("/api/component_readiness/regressions/%d", wouldClose.ID), &detail)) + assert.True(t, detail.ForceClosed, "regression detail should report force_closed") + require.NotNil(t, detail.ForceClosedByTriageID) + assert.Equal(t, triageResp.ID, *detail.ForceClosedByTriageID) + require.NotNil(t, detail.ForceClosedBy, "detail should include force_closed_by directly from the regression") + assert.Equal(t, "developer", *detail.ForceClosedBy) + require.NotNil(t, detail.ForceClosedReason, "detail should include force_closed_reason directly from the regression") + assert.Equal(t, "generic test, unrelated failures", *detail.ForceClosedReason) + + // The regression opened after the resolution time was left open. + var openReg models.TestRegression + require.NoError(t, dbc.DB.First(&openReg, wouldNotClose.ID).Error) + assert.False(t, openReg.Closed.Valid, "regression opened after resolution should remain open") + assert.False(t, openReg.ForceClosed, "regression opened after resolution should not be force closed") }) } diff --git a/test/integration/regression_forceclose_test.go b/test/integration/regression_forceclose_test.go new file mode 100644 index 0000000000..07c8918720 --- /dev/null +++ b/test/integration/regression_forceclose_test.go @@ -0,0 +1,317 @@ +package integration + +import ( + "database/sql" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "gorm.io/gorm" + + "github.com/openshift/sippy/pkg/api/componentreadiness" + "github.com/openshift/sippy/pkg/db/models" + intutil "github.com/openshift/sippy/test/integration/util" +) + +// These tests exercise the force close regression store methods against a real PostgreSQL clone. +// pgContainer and TestMain live in jobs_test.go (same package), so each test just calls +// intutil.NewTestDB(t, pgContainer) for an isolated database. + +func TestForceCloseRegressions(t *testing.T) { + t.Run("basic close records force close metadata on the regression", func(t *testing.T) { + dbc := intutil.NewTestDB(t, pgContainer) + store := componentreadiness.NewPostgresRegressionStore(dbc, nil) + + resolved := time.Now().Truncate(time.Second) + reg := intutil.CreateTestRegression(t, dbc, "basic-close", "4.19", + intutil.WithOpened(resolved.Add(-10*24*time.Hour))) + triage := intutil.CreateTriage(t, dbc, "https://issues.example.com/BASIC-1", + intutil.WithResolved(resolved), intutil.WithRegressions(reg)) + + result, err := store.ForceCloseRegressions(triage.ID, "developer", "unrelated failures") + require.NoError(t, err) + require.NotNil(t, result) + assert.ElementsMatch(t, []uint{reg.ID}, result.ClosedRegressionIDs) + assert.WithinDuration(t, resolved, result.Timestamp, time.Second, "close time should be the resolution time") + + var got models.TestRegression + require.NoError(t, dbc.DB.First(&got, reg.ID).Error) + assert.True(t, got.Closed.Valid, "regression should be closed") + assert.WithinDuration(t, resolved, got.Closed.Time, time.Second, "regression should close at the resolution time") + assert.True(t, got.ForceClosed) + require.NotNil(t, got.ForceClosedBy) + assert.Equal(t, "developer", *got.ForceClosedBy) + require.NotNil(t, got.ForceClosedReason) + assert.Equal(t, "unrelated failures", *got.ForceClosedReason) + require.NotNil(t, got.ForceClosedByTriageID) + assert.Equal(t, triage.ID, *got.ForceClosedByTriageID) + }) + + t.Run("time scoping closes only regressions opened before the resolution time", func(t *testing.T) { + dbc := intutil.NewTestDB(t, pgContainer) + store := componentreadiness.NewPostgresRegressionStore(dbc, nil) + + resolved := time.Now().Add(-5 * 24 * time.Hour).Truncate(time.Second) + before := intutil.CreateTestRegression(t, dbc, "scope-before", "4.19", + intutil.WithOpened(resolved.Add(-24*time.Hour))) + // Opened at the exact resolution instant: the exclusive boundary (opened < resolved) means it + // must NOT be closed. + atBoundary := intutil.CreateTestRegression(t, dbc, "scope-boundary", "4.19", + intutil.WithOpened(resolved)) + after := intutil.CreateTestRegression(t, dbc, "scope-after", "4.19", + intutil.WithOpened(resolved.Add(24*time.Hour))) + triage := intutil.CreateTriage(t, dbc, "https://issues.example.com/SCOPE-1", + intutil.WithResolved(resolved), intutil.WithRegressions(before, atBoundary, after)) + + result, err := store.ForceCloseRegressions(triage.ID, "developer", "scoped close") + require.NoError(t, err) + assert.ElementsMatch(t, []uint{before.ID}, result.ClosedRegressionIDs, + "only the regression opened before the resolution time should be closed") + + var gotBefore, gotBoundary, gotAfter models.TestRegression + require.NoError(t, dbc.DB.First(&gotBefore, before.ID).Error) + require.NoError(t, dbc.DB.First(&gotBoundary, atBoundary.ID).Error) + require.NoError(t, dbc.DB.First(&gotAfter, after.ID).Error) + + assert.True(t, gotBefore.ForceClosed, "regression opened before resolution should be force closed") + assert.WithinDuration(t, resolved, gotBefore.Closed.Time, time.Second) + + assert.False(t, gotBoundary.Closed.Valid, "regression opened exactly at resolution should remain open") + assert.False(t, gotBoundary.ForceClosed, "boundary regression should not be force closed") + + assert.False(t, gotAfter.Closed.Valid, "regression opened after resolution should remain open") + assert.False(t, gotAfter.ForceClosed) + }) + + t.Run("unresolved triage returns ErrTriageNotResolved and touches nothing", func(t *testing.T) { + dbc := intutil.NewTestDB(t, pgContainer) + store := componentreadiness.NewPostgresRegressionStore(dbc, nil) + + reg := intutil.CreateTestRegression(t, dbc, "guard-open", "4.19", + intutil.WithOpened(time.Now().Add(-10*24*time.Hour))) + triage := intutil.CreateTriage(t, dbc, "https://issues.example.com/GUARD-1", intutil.WithRegressions(reg)) + + _, err := store.ForceCloseRegressions(triage.ID, "developer", "should fail") + require.Error(t, err) + assert.ErrorIs(t, err, componentreadiness.ErrTriageNotResolved) + + var got models.TestRegression + require.NoError(t, dbc.DB.First(&got, reg.ID).Error) + assert.False(t, got.Closed.Valid, "regression should remain open") + assert.False(t, got.ForceClosed, "regression should not be force closed") + }) + + t.Run("idempotent repeat call closes nothing new and leaves the close time unchanged", func(t *testing.T) { + dbc := intutil.NewTestDB(t, pgContainer) + store := componentreadiness.NewPostgresRegressionStore(dbc, nil) + + resolved := time.Now().Truncate(time.Second) + reg := intutil.CreateTestRegression(t, dbc, "idem", "4.19", + intutil.WithOpened(resolved.Add(-10*24*time.Hour))) + triage := intutil.CreateTriage(t, dbc, "https://issues.example.com/IDEM-1", + intutil.WithResolved(resolved), intutil.WithRegressions(reg)) + + first, err := store.ForceCloseRegressions(triage.ID, "developer", "first call") + require.NoError(t, err) + require.Len(t, first.ClosedRegressionIDs, 1) + + var afterFirst models.TestRegression + require.NoError(t, dbc.DB.First(&afterFirst, reg.ID).Error) + originalClose := afterFirst.Closed.Time + + second, err := store.ForceCloseRegressions(triage.ID, "developer", "second call") + require.NoError(t, err) + assert.Empty(t, second.ClosedRegressionIDs, "repeat call should close no additional regressions") + + var afterSecond models.TestRegression + require.NoError(t, dbc.DB.First(&afterSecond, reg.ID).Error) + assert.WithinDuration(t, originalClose, afterSecond.Closed.Time, time.Second, + "closed time should not change on a repeat call") + }) +} + +func TestForceClosePreview(t *testing.T) { + t.Run("classifies would-close and would-not-close with failure gap data", func(t *testing.T) { + dbc := intutil.NewTestDB(t, pgContainer) + store := componentreadiness.NewPostgresRegressionStore(dbc, nil) + + resolved := time.Now().Add(-5 * 24 * time.Hour).Truncate(time.Second) + + wouldClose := intutil.CreateTestRegression(t, dbc, "prev-close", "4.19", + intutil.WithOpened(resolved.Add(-10*24*time.Hour))) + // Opened exactly at the resolution instant belongs in would_not_close (exclusive boundary). + atBoundary := intutil.CreateTestRegression(t, dbc, "prev-boundary", "4.19", + intutil.WithOpened(resolved)) + wouldNotClose := intutil.CreateTestRegression(t, dbc, "prev-open", "4.19", + intutil.WithOpened(resolved.Add(24*time.Hour))) + triage := intutil.CreateTriage(t, dbc, "https://issues.example.com/PREV-1", + intutil.WithResolved(resolved), intutil.WithRegressions(wouldClose, atBoundary, wouldNotClose)) + + // Failing runs before and after the resolution time drive the gap indicator on the closing regression. + lastBefore := resolved.Add(-2 * 24 * time.Hour) + firstAfter := resolved.Add(2 * 24 * time.Hour) + require.NoError(t, dbc.DB.Create(&models.RegressionJobRun{ + RegressionID: wouldClose.ID, ProwJobRunID: "gap-before", ProwJobName: "job-1", StartTime: lastBefore, TestFailed: true, + }).Error) + require.NoError(t, dbc.DB.Create(&models.RegressionJobRun{ + RegressionID: wouldClose.ID, ProwJobRunID: "gap-after", ProwJobName: "job-1", StartTime: firstAfter, TestFailed: true, + }).Error) + + preview, err := store.ForceClosePreview(triage.ID) + require.NoError(t, err) + assert.WithinDuration(t, resolved, preview.Resolved, time.Second) + + require.Len(t, preview.WouldClose, 1, "only the regression opened before resolution should be in would_close") + assert.Equal(t, wouldClose.ID, preview.WouldClose[0].RegressionID) + require.NotNil(t, preview.WouldClose[0].LastFailureBeforeResolution, "should report the last failure before resolution") + assert.WithinDuration(t, lastBefore, *preview.WouldClose[0].LastFailureBeforeResolution, time.Second) + require.NotNil(t, preview.WouldClose[0].FirstFailureAfterResolution, "should report the first failure after resolution") + assert.WithinDuration(t, firstAfter, *preview.WouldClose[0].FirstFailureAfterResolution, time.Second) + + notCloseIDs := make([]uint, 0, len(preview.WouldNotClose)) + for _, r := range preview.WouldNotClose { + notCloseIDs = append(notCloseIDs, r.RegressionID) + } + assert.ElementsMatch(t, []uint{atBoundary.ID, wouldNotClose.ID}, notCloseIDs, + "regressions opened at or after resolution should be in would_not_close") + }) + + t.Run("unresolved triage returns ErrTriageNotResolved", func(t *testing.T) { + dbc := intutil.NewTestDB(t, pgContainer) + store := componentreadiness.NewPostgresRegressionStore(dbc, nil) + + reg := intutil.CreateTestRegression(t, dbc, "prev-guard", "4.19") + triage := intutil.CreateTriage(t, dbc, "https://issues.example.com/PREV-GUARD", intutil.WithRegressions(reg)) + + _, err := store.ForceClosePreview(triage.ID) + assert.ErrorIs(t, err, componentreadiness.ErrTriageNotResolved) + }) +} + +func TestListCurrentRegressionsForReleaseForceClose(t *testing.T) { + t.Run("force closed regressions are excluded from the reuse window", func(t *testing.T) { + dbc := intutil.NewTestDB(t, pgContainer) + store := componentreadiness.NewPostgresRegressionStore(dbc, nil) + + now := time.Now() + recentlyClosed := now.Add(-24 * time.Hour) // within the 5-day hysteresis reuse window + by := "developer" + reason := "excluded from reuse" + + open := intutil.CreateTestRegression(t, dbc, "list-open", "4.19", + intutil.WithOpened(now.Add(-10*24*time.Hour))) + recentNormal := intutil.CreateTestRegression(t, dbc, "list-recent-normal", "4.19", + intutil.WithOpened(now.Add(-10*24*time.Hour)), intutil.WithClosed(recentlyClosed)) + forceClosed := intutil.CreateTestRegression(t, dbc, "list-forceclosed", "4.19", + intutil.WithOpened(now.Add(-10*24*time.Hour)), intutil.WithClosed(recentlyClosed), + intutil.WithForceClosed(true), intutil.WithForceClosedBy(&by), intutil.WithForceClosedReason(&reason)) + oldClosed := intutil.CreateTestRegression(t, dbc, "list-old-closed", "4.19", + intutil.WithOpened(now.Add(-30*24*time.Hour)), intutil.WithClosed(now.Add(-10*24*time.Hour))) + otherRelease := intutil.CreateTestRegression(t, dbc, "list-other-release", "4.18", + intutil.WithOpened(now.Add(-24*time.Hour))) + + got, err := store.ListCurrentRegressionsForRelease("4.19") + require.NoError(t, err) + + gotIDs := make([]uint, 0, len(got)) + for _, r := range got { + gotIDs = append(gotIDs, r.ID) + } + assert.Contains(t, gotIDs, open.ID, "open regression should be listed") + assert.Contains(t, gotIDs, recentNormal.ID, "recently closed (non-force) regression should be within the reuse window") + assert.NotContains(t, gotIDs, forceClosed.ID, "force closed regression should be excluded even though it closed recently") + assert.NotContains(t, gotIDs, oldClosed.ID, "regression closed beyond the hysteresis window should be excluded") + assert.NotContains(t, gotIDs, otherRelease.ID, "regression from another release should be excluded") + }) +} + +func TestResolveTriagesForceClose(t *testing.T) { + t.Run("force closed regressions do not block triage auto-resolution", func(t *testing.T) { + dbc := intutil.NewTestDB(t, pgContainer) + store := componentreadiness.NewPostgresRegressionStore(dbc, nil) + + // Closed recently, well within the 5-day hysteresis window: a non-force close would still block + // auto-resolution, but a force close should not. + recentlyClosed := time.Now().Add(-1 * time.Hour).Truncate(time.Second) + + // Triage A: its only regression was force closed. Force closed regressions no longer count as + // active, so the triage should auto-resolve despite the recent close. + forceClosedReg := intutil.CreateTestRegression(t, dbc, "resolve-forceclosed", "4.19", + intutil.WithOpened(time.Now().Add(-10*24*time.Hour))) + triageA := intutil.CreateTriage(t, dbc, "https://issues.example.com/RESOLVE-A", + intutil.WithRegressions(forceClosedReg)) + require.NoError(t, dbc.DB.Model(&models.TestRegression{}).Where("id = ?", forceClosedReg.ID). + Updates(map[string]any{ + "closed": sql.NullTime{Valid: true, Time: recentlyClosed}, + "force_closed": true, + "force_closed_by": "developer", + "force_closed_reason": "force closed", + }).Error) + + // Triage B: its only regression was closed recently but NOT force closed, so it still blocks + // auto-resolution until the hysteresis window passes. + normalReg := intutil.CreateTestRegression(t, dbc, "resolve-normal", "4.19", + intutil.WithOpened(time.Now().Add(-10*24*time.Hour))) + triageB := intutil.CreateTriage(t, dbc, "https://issues.example.com/RESOLVE-B", + intutil.WithRegressions(normalReg)) + require.NoError(t, dbc.DB.Model(&models.TestRegression{}).Where("id = ?", normalReg.ID). + Update("closed", sql.NullTime{Valid: true, Time: recentlyClosed}).Error) + + require.NoError(t, store.ResolveTriages()) + + var gotA, gotB models.Triage + require.NoError(t, dbc.DB.First(&gotA, triageA.ID).Error) + require.NoError(t, dbc.DB.First(&gotB, triageB.ID).Error) + + assert.True(t, gotA.Resolved.Valid, "triage with only a force closed regression should be auto-resolved") + assert.WithinDuration(t, recentlyClosed, gotA.Resolved.Time, time.Second, + "resolution time should match the regression close time") + assert.False(t, gotB.Resolved.Valid, + "triage with a recently, non-force closed regression should still be blocked") + }) +} + +func TestForceCloseRegressionsErrorPaths(t *testing.T) { + t.Run("nonexistent triage returns a not found error", func(t *testing.T) { + dbc := intutil.NewTestDB(t, pgContainer) + store := componentreadiness.NewPostgresRegressionStore(dbc, nil) + + _, err := store.ForceCloseRegressions(999999, "developer", "valid reason") + require.Error(t, err) + assert.ErrorIs(t, err, gorm.ErrRecordNotFound) + }) + + t.Run("empty reason is rejected and closes nothing", func(t *testing.T) { + dbc := intutil.NewTestDB(t, pgContainer) + store := componentreadiness.NewPostgresRegressionStore(dbc, nil) + + resolved := time.Now().Truncate(time.Second) + reg := intutil.CreateTestRegression(t, dbc, "empty-reason", "4.19", + intutil.WithOpened(resolved.Add(-24*time.Hour))) + triage := intutil.CreateTriage(t, dbc, "https://issues.example.com/REASON-EMPTY", + intutil.WithResolved(resolved), intutil.WithRegressions(reg)) + + _, err := store.ForceCloseRegressions(triage.ID, "developer", "") + assert.ErrorIs(t, err, componentreadiness.ErrForceCloseReasonRequired) + + var got models.TestRegression + require.NoError(t, dbc.DB.First(&got, reg.ID).Error) + assert.False(t, got.Closed.Valid, "regression must not be closed when the reason is rejected") + assert.False(t, got.ForceClosed) + }) + + t.Run("whitespace only reason is rejected", func(t *testing.T) { + dbc := intutil.NewTestDB(t, pgContainer) + store := componentreadiness.NewPostgresRegressionStore(dbc, nil) + + resolved := time.Now().Truncate(time.Second) + reg := intutil.CreateTestRegression(t, dbc, "ws-reason", "4.19", + intutil.WithOpened(resolved.Add(-24*time.Hour))) + triage := intutil.CreateTriage(t, dbc, "https://issues.example.com/REASON-WS", + intutil.WithResolved(resolved), intutil.WithRegressions(reg)) + + _, err := store.ForceCloseRegressions(triage.ID, "developer", " ") + assert.ErrorIs(t, err, componentreadiness.ErrForceCloseReasonRequired) + }) +} diff --git a/test/integration/util/fixtures.go b/test/integration/util/fixtures.go index 8c28f1477c..fadba71e42 100644 --- a/test/integration/util/fixtures.go +++ b/test/integration/util/fixtures.go @@ -1,6 +1,8 @@ package util import ( + "context" + "database/sql" "fmt" "testing" "time" @@ -365,3 +367,101 @@ func LinkReleaseTagPullRequests(t *testing.T, dbc *db.DB, tag *models.ReleaseTag } require.NoError(t, dbc.DB.Model(tag).Association("PullRequests").Append(prPtrs), "linking PRs to ReleaseTag %q", tag.ReleaseTag) } + +// TestRegressionOption customizes a TestRegression before creation. +type TestRegressionOption func(*models.TestRegression) + +// WithOpened sets the regression's opened time. Force close scopes on this, closing only regressions +// that opened strictly before the triage's resolution time. +func WithOpened(opened time.Time) TestRegressionOption { + return func(r *models.TestRegression) { r.Opened = opened } +} + +// WithClosed marks the regression closed at the given time. +func WithClosed(closed time.Time) TestRegressionOption { + return func(r *models.TestRegression) { r.Closed = sql.NullTime{Valid: true, Time: closed} } +} + +// WithForceClosed sets the force_closed flag directly, for seeding already-force-closed regressions. +func WithForceClosed(forceClosed bool) TestRegressionOption { + return func(r *models.TestRegression) { r.ForceClosed = forceClosed } +} + +// WithForceClosedBy sets who force closed the regression. +func WithForceClosedBy(by *string) TestRegressionOption { + return func(r *models.TestRegression) { r.ForceClosedBy = by } +} + +// WithForceClosedReason sets the recorded force close reason. +func WithForceClosedReason(reason *string) TestRegressionOption { + return func(r *models.TestRegression) { r.ForceClosedReason = reason } +} + +// CreateTestRegression creates a test_regressions row with the required non-null fields populated +// (TestName, TestID, Release, and a non-null Variants array), defaulting Opened to now. Callers +// override any field with the With* options. +func CreateTestRegression(t *testing.T, dbc *db.DB, testName, release string, opts ...TestRegressionOption) *models.TestRegression { + t.Helper() + reg := &models.TestRegression{ + TestName: testName, + TestID: testName + "-id", + Release: release, + Variants: pq.StringArray{"variant:integration"}, + Opened: time.Now(), + } + for _, opt := range opts { + opt(reg) + } + require.NoError(t, dbc.DB.Create(reg).Error, "creating TestRegression %q", testName) + return reg +} + +// triageConfig collects the desired triage state plus the regressions to associate after creation. +type triageConfig struct { + triage models.Triage + regressions []*models.TestRegression +} + +// TriageOption customizes a Triage before creation. +type TriageOption func(*triageConfig) + +// WithResolved marks the triage resolved at the given time, giving force close a resolution time to +// scope against. +func WithResolved(resolved time.Time) TriageOption { + return func(c *triageConfig) { c.triage.Resolved = sql.NullTime{Valid: true, Time: resolved} } +} + +// WithTriageType overrides the triage type (defaults to product). +func WithTriageType(triageType string) TriageOption { + return func(c *triageConfig) { c.triage.Type = models.TriageType(triageType) } +} + +// WithRegressions links the given existing regressions to the triage. The link is made via the GORM +// Association API after the triage is created, never by embedding the regressions in the Create call: +// embedding makes GORM upsert them, rewriting partial in-memory objects (for example nil Variants) and +// violating the NOT NULL constraint on variants. +func WithRegressions(regs ...*models.TestRegression) TriageOption { + return func(c *triageConfig) { c.regressions = append(c.regressions, regs...) } +} + +// CreateTriage creates a triage row and links any requested regressions via the Association API. The +// triage is created under a user context so the audit-log AfterCreate hook has a user to attribute. +func CreateTriage(t *testing.T, dbc *db.DB, url string, opts ...TriageOption) models.Triage { + t.Helper() + cfg := &triageConfig{ + triage: models.Triage{ + URL: url, + Type: models.TriageTypeProduct, + }, + } + for _, opt := range opts { + opt(cfg) + } + dbCtx := dbc.DB.WithContext(context.WithValue(context.Background(), models.CurrentUserKey, "integration-test")) + require.NoError(t, dbCtx.Create(&cfg.triage).Error, "creating Triage %q", url) + if len(cfg.regressions) > 0 { + require.NoError(t, dbCtx.Model(&cfg.triage).Association("Regressions").Append(cfg.regressions), + "linking regressions to Triage %q", url) + } + return cfg.triage +} From a9599901ce96483a03d09e3532c40feebd7e3d84 Mon Sep 17 00:00:00 2001 From: Chai Bot Date: Thu, 20 Aug 2026 02:27:43 +0000 Subject: [PATCH 09/10] TRT-2895: Treat missing triage records as clean state in e2e cleanup cleanupTriages now filters out gorm.ErrRecordNotFound on each delete step so a deferred cleanup with nothing to remove is treated as success rather than surfacing a spurious error. Applied to both the triage_regressions join-table delete and the triage delete. Co-Authored-By: Claude Opus 4.8 --- .../regressiontracker/regressiontracker_test.go | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/test/e2e/componentreadiness/regressiontracker/regressiontracker_test.go b/test/e2e/componentreadiness/regressiontracker/regressiontracker_test.go index bdb9838021..c2381dca29 100644 --- a/test/e2e/componentreadiness/regressiontracker/regressiontracker_test.go +++ b/test/e2e/componentreadiness/regressiontracker/regressiontracker_test.go @@ -3,6 +3,7 @@ package regressiontracker import ( "context" "database/sql" + "errors" "fmt" "testing" "time" @@ -21,6 +22,7 @@ import ( log "github.com/sirupsen/logrus" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "gorm.io/gorm" "k8s.io/apimachinery/pkg/util/sets" ) @@ -603,10 +605,12 @@ func Test_RegressionJobRuns(t *testing.T) { // referenced row. It returns an error rather than only logging so a deferred cleanup can assert on it and // surface leaks instead of silently letting rows bleed into later tests. func cleanupTriages(dbc *db.DB) error { - if err := dbc.DB.Exec("DELETE FROM triage_regressions WHERE 1=1").Error; err != nil { + // gorm.ErrRecordNotFound simply means there was nothing to delete, which is a valid + // cleanup state rather than a failure, so it is filtered out on each step. + if err := dbc.DB.Exec("DELETE FROM triage_regressions WHERE 1=1").Error; err != nil && !errors.Is(err, gorm.ErrRecordNotFound) { return fmt.Errorf("error deleting triage_regressions: %w", err) } - if err := dbc.DB.Where("1 = 1").Delete(&models.Triage{}).Error; err != nil { + if err := dbc.DB.Where("1 = 1").Delete(&models.Triage{}).Error; err != nil && !errors.Is(err, gorm.ErrRecordNotFound) { return fmt.Errorf("error deleting triage records: %w", err) } return nil From 96bfb7a25322a5c26e241143f3c4a94ee234e142 Mon Sep 17 00:00:00 2001 From: Chai Bot Date: Sat, 22 Aug 2026 04:31:19 +0000 Subject: [PATCH 10/10] TRT-2895: Use test_failures > 0 in force-close gap query The regression_job_runs.test_failed boolean is never populated in production (all rows are false), so the force-close failure-gap query matched nothing. Switch both queries in queryRegressionFailureGaps to filter on the test_failures integer count, which is populated correctly. Update the force-close preview fixtures (integration and e2e) to set TestFailures alongside TestFailed so the gap query has matching rows. Co-Authored-By: Claude Opus 4.8 --- pkg/api/componentreadiness/regressiontracker.go | 4 ++-- test/e2e/componentreadiness/triage/triageapi_test.go | 4 ++-- test/integration/regression_forceclose_test.go | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/pkg/api/componentreadiness/regressiontracker.go b/pkg/api/componentreadiness/regressiontracker.go index fe4e884b26..727d420f4f 100644 --- a/pkg/api/componentreadiness/regressiontracker.go +++ b/pkg/api/componentreadiness/regressiontracker.go @@ -423,7 +423,7 @@ func (prs *PostgresRegressionStore) queryRegressionFailureGaps(regressionIDs []u var lastRows []gapRow if err := prs.dbc.DB.Table("regression_job_runs"). Select("regression_id, MAX(start_time) AS failure_time"). - Where("regression_id IN ? AND start_time <= ? AND test_failed = true", regressionIDs, resolutionTime). + Where("regression_id IN ? AND start_time <= ? AND test_failures > 0", regressionIDs, resolutionTime). Group("regression_id"). Scan(&lastRows).Error; err != nil { return nil, fmt.Errorf("error querying last failures before resolution for regressions %v: %w", regressionIDs, err) @@ -441,7 +441,7 @@ func (prs *PostgresRegressionStore) queryRegressionFailureGaps(regressionIDs []u var firstRows []gapRow if err := prs.dbc.DB.Table("regression_job_runs"). Select("regression_id, MIN(start_time) AS failure_time"). - Where("regression_id IN ? AND start_time > ? AND test_failed = true", regressionIDs, resolutionTime). + Where("regression_id IN ? AND start_time > ? AND test_failures > 0", regressionIDs, resolutionTime). Group("regression_id"). Scan(&firstRows).Error; err != nil { return nil, fmt.Errorf("error querying first failures after resolution for regressions %v: %w", regressionIDs, err) diff --git a/test/e2e/componentreadiness/triage/triageapi_test.go b/test/e2e/componentreadiness/triage/triageapi_test.go index 615f9ef077..0bce8bfc1f 100644 --- a/test/e2e/componentreadiness/triage/triageapi_test.go +++ b/test/e2e/componentreadiness/triage/triageapi_test.go @@ -816,8 +816,8 @@ func Test_ForceCloseRegressionsAPI(t *testing.T) { // Failures before and after the resolution time drive the preview gap indicator. require.NoError(t, tracker.MergeJobRuns(wouldClose.ID, []models.RegressionJobRun{ - {ProwJobRunID: "api-before", ProwJobName: "job-1", StartTime: resolved.Add(-2 * 24 * time.Hour), TestFailed: true}, - {ProwJobRunID: "api-after", ProwJobName: "job-1", StartTime: resolved.Add(2 * 24 * time.Hour), TestFailed: true}, + {ProwJobRunID: "api-before", ProwJobName: "job-1", StartTime: resolved.Add(-2 * 24 * time.Hour), TestFailed: true, TestFailures: 1}, + {ProwJobRunID: "api-after", ProwJobName: "job-1", StartTime: resolved.Add(2 * 24 * time.Hour), TestFailed: true, TestFailures: 1}, })) // Create the triage over both regressions, then resolve it via the API. diff --git a/test/integration/regression_forceclose_test.go b/test/integration/regression_forceclose_test.go index 07c8918720..9189346b05 100644 --- a/test/integration/regression_forceclose_test.go +++ b/test/integration/regression_forceclose_test.go @@ -152,10 +152,10 @@ func TestForceClosePreview(t *testing.T) { lastBefore := resolved.Add(-2 * 24 * time.Hour) firstAfter := resolved.Add(2 * 24 * time.Hour) require.NoError(t, dbc.DB.Create(&models.RegressionJobRun{ - RegressionID: wouldClose.ID, ProwJobRunID: "gap-before", ProwJobName: "job-1", StartTime: lastBefore, TestFailed: true, + RegressionID: wouldClose.ID, ProwJobRunID: "gap-before", ProwJobName: "job-1", StartTime: lastBefore, TestFailed: true, TestFailures: 1, }).Error) require.NoError(t, dbc.DB.Create(&models.RegressionJobRun{ - RegressionID: wouldClose.ID, ProwJobRunID: "gap-after", ProwJobName: "job-1", StartTime: firstAfter, TestFailed: true, + RegressionID: wouldClose.ID, ProwJobRunID: "gap-after", ProwJobName: "job-1", StartTime: firstAfter, TestFailed: true, TestFailures: 1, }).Error) preview, err := store.ForceClosePreview(triage.ID)