From 0b26e9936930b398fc1b74d37eceff00d99c276b Mon Sep 17 00:00:00 2001 From: Dennis Wuitz Date: Tue, 26 May 2026 20:02:51 +0200 Subject: [PATCH] fix(web): surface 4xx + UI banner when evaluation start fails (#280) --- backend/core/src/sources/git.rs | 12 +--- backend/core/tests/git_remote.rs | 48 ++++++++++++++++ .../web/src/endpoints/projects/evaluations.rs | 15 +++-- .../web/src/endpoints/projects/management.rs | 23 ++++---- backend/web/src/error.rs | 1 + docs/gradient-api.yaml | 30 +++++++++- docs/src/tests.md | 30 ++++++++++ .../project-detail.component.html | 14 +++++ .../project-detail.component.scss | 37 ++++++++++++ .../project-detail.component.spec.ts | 56 ++++++++++++++++++- .../project-detail.component.ts | 9 +++ 11 files changed, 243 insertions(+), 32 deletions(-) create mode 100644 backend/core/tests/git_remote.rs diff --git a/backend/core/src/sources/git.rs b/backend/core/src/sources/git.rs index 6dfc2190..11f69edd 100644 --- a/backend/core/src/sources/git.rs +++ b/backend/core/src/sources/git.rs @@ -12,7 +12,7 @@ use async_trait::async_trait; use git2::{Direction, RemoteCallbacks}; use sea_orm::EntityTrait; use std::sync::Arc; -use tracing::{debug, info, instrument, warn}; +use tracing::{debug, info, instrument}; // ── ProjectGitContext ───────────────────────────────────────────────────────── @@ -79,7 +79,7 @@ impl<'a> ProjectGitContext<'a> { let ssh_creds = self.ssh_creds.clone(); let branch_owned = branch.map(|b| b.to_owned()); - let remote_hash = match tokio::task::spawn_blocking(move || { + let remote_hash = tokio::task::spawn_blocking(move || { let branch_ref = branch_owned.as_deref(); if let Some((private_key, public_key)) = ssh_creds { ls_remote_head(&url, Some(&private_key), Some(&public_key), branch_ref) @@ -90,13 +90,7 @@ impl<'a> ProjectGitContext<'a> { .await .map_err(|e| SourceError::GitExecution { error: e.to_string(), - })? { - Ok(hash) => hash, - Err(e) => { - warn!(error = %e, "Failed to get remote HEAD ref, will retry next cycle"); - return Ok((false, vec![])); - } - }; + })??; let remote_hash_str = vec_to_hex(&remote_hash); debug!(remote_hash = %remote_hash_str, "Retrieved remote hash"); diff --git a/backend/core/tests/git_remote.rs b/backend/core/tests/git_remote.rs new file mode 100644 index 00000000..43eae84b --- /dev/null +++ b/backend/core/tests/git_remote.rs @@ -0,0 +1,48 @@ +/* + * SPDX-FileCopyrightText: 2026 Wavelens GmbH + * + * SPDX-License-Identifier: AGPL-3.0-only + */ + +//! Tests for `check_project_updates` error propagation. +//! +//! Issue #280: manual `POST /evaluate` used to swallow git fetch failures +//! (DNS, connection refused, …) and bubble up a generic 500. The fix +//! propagates the `SourceError` to the caller so the web layer can return +//! a 4xx with a useful message. + +extern crate core as gradient_core; + +use entity::project; +use gradient_core::sources::check_project_updates; +use sea_orm::{DatabaseBackend, MockDatabase}; +use test_support::state::test_state; + +/// `git://` URLs use the pure-Rust pkt-line path, so `TcpStream::connect` +/// to an unbound loopback port returns "connection refused" without +/// touching the network beyond loopback. Before the fix, this case was +/// hidden behind `Ok((false, vec![]))`. +#[test] +fn check_project_updates_propagates_unreachable_remote_error() { + let rt = tokio::runtime::Builder::new_current_thread() + .enable_all() + .build() + .unwrap(); + rt.block_on(async { + let db = MockDatabase::new(DatabaseBackend::Postgres).into_connection(); + let state = test_state(db); + let project = project::Model { + repository: "git://127.0.0.1:1/nonexistent.git".into(), + force_evaluation: true, + ..Default::default() + }; + + let result = check_project_updates(state, &project, None).await; + + assert!( + result.is_err(), + "expected Err for unreachable git:// URL, got {:?}", + result + ); + }); +} diff --git a/backend/web/src/endpoints/projects/evaluations.rs b/backend/web/src/endpoints/projects/evaluations.rs index 272833c3..b6bf27f3 100644 --- a/backend/web/src/endpoints/projects/evaluations.rs +++ b/backend/web/src/endpoints/projects/evaluations.rs @@ -10,7 +10,7 @@ use super::{ use crate::access::{Caller, ProjectAccess, has_permission, is_org_member, load_project}; use crate::authorization::{MaybeApiKey, MaybeUser}; use crate::endpoints::content_type_for_filename; -use crate::error::{WebError, WebResult}; +use crate::error::{ErrorCode, WebError, WebResult}; use crate::helpers::{OptionExt, ok_json}; use crate::permissions::Permission; use axum::extract::{Path, Query, State}; @@ -213,13 +213,12 @@ pub async fn post_project_evaluate( let (_has_updates, commit_hash) = check_project_updates(Arc::clone(&state), &project_for_check, None) .await - .map_err(|e| anyhow::anyhow!(e))?; - - if commit_hash.is_empty() { - return Err(WebError::internal( - "Failed to fetch repository state".to_string(), - )); - } + .map_err(|e| { + WebError::bad_request_with( + ErrorCode::REPOSITORY_UNREACHABLE, + format!("Failed to fetch repository state: {}", e), + ) + })?; let eval = gradient_core::ci::trigger_evaluation( &state.web_db, diff --git a/backend/web/src/endpoints/projects/management.rs b/backend/web/src/endpoints/projects/management.rs index 3589e51c..764df9df 100644 --- a/backend/web/src/endpoints/projects/management.rs +++ b/backend/web/src/endpoints/projects/management.rs @@ -8,7 +8,7 @@ use super::ProjectResponse; use crate::access::{Caller, OrgAccess, ProjectAccess, has_permission, load_org, load_project}; use crate::audit::{RequestInfo, events, record as audit_record}; use crate::authorization::{MaybeApiKey, MaybeUser}; -use crate::error::{WebError, WebResult}; +use crate::error::{ErrorCode, WebError, WebResult}; use crate::helpers::{OptionExt, ok_json}; use crate::permissions::Permission; use axum::extract::{Path, Query, State}; @@ -629,18 +629,19 @@ pub async fn post_project_check_repository( let (_has_updates, remote_hash) = check_project_updates(Arc::clone(&state), &project, None) .await - .map_err(|e| anyhow::anyhow!(e))?; + .map_err(|e| { + WebError::bad_request_with( + ErrorCode::REPOSITORY_UNREACHABLE, + format!("Failed to check repository: {}", e), + ) + })?; - if !remote_hash.is_empty() { - let res = BaseResponse { - error: false, - message: vec_to_hex(&remote_hash), - }; + let res = BaseResponse { + error: false, + message: vec_to_hex(&remote_hash), + }; - Ok(Json(res)) - } else { - Err(WebError::internal("Failed to check repository".to_string())) - } + Ok(Json(res)) } pub async fn post_project_transfer( diff --git a/backend/web/src/error.rs b/backend/web/src/error.rs index 84f2cb6e..7efbf082 100644 --- a/backend/web/src/error.rs +++ b/backend/web/src/error.rs @@ -46,6 +46,7 @@ impl ErrorCode { pub const INVALID_OAUTH_CODE: Self = Self("invalid_oauth_code"); pub const OAUTH_DISABLED: Self = Self("oauth_disabled"); pub const REGISTRATION_DISABLED: Self = Self("registration_disabled"); + pub const REPOSITORY_UNREACHABLE: Self = Self("repository_unreachable"); // 401 Unauthorized pub const UNAUTHORIZED: Self = Self("unauthorized"); diff --git a/docs/gradient-api.yaml b/docs/gradient-api.yaml index 2d51755b..4cb16e93 100644 --- a/docs/gradient-api.yaml +++ b/docs/gradient-api.yaml @@ -1744,6 +1744,9 @@ paths: Tests whether Gradient can clone the project's repository using the organization's SSH key. Requires Admin or Write role; rejects state-managed projects. + + Returns `400` with `code: "repository_unreachable"` and the underlying + git error when the repository cannot be fetched. operationId: checkProjectRepository responses: '200': @@ -1753,7 +1756,13 @@ paths: schema: $ref: '#/components/schemas/StringResponse' '400': - $ref: '#/components/responses/BadRequest' + description: >- + Bad request - invalid input or repository unreachable + (`code: "repository_unreachable"`). + content: + application/json: + schema: + $ref: '#/components/schemas/StringResponse' '401': $ref: '#/components/responses/Unauthorized' '403': @@ -1768,7 +1777,12 @@ paths: post: tags: [projects] summary: Trigger evaluation - description: Queues a new evaluation for the project. Returns the new evaluation UUID. + description: >- + Queues a new evaluation for the project. Returns the new evaluation UUID. + + Returns `400` with `code: "repository_unreachable"` when the repository + cannot be fetched (DNS failure, authentication failure, unreachable host, + bad URL, …) - the message includes the underlying git error. operationId: triggerEvaluation responses: '200': @@ -1781,7 +1795,17 @@ paths: error: false message: "3fa85f64-5717-4562-b3fc-2c963f66afa6" '400': - $ref: '#/components/responses/BadRequest' + description: >- + Bad request - invalid input, evaluation already in progress, or + repository unreachable (`code: "repository_unreachable"`). + content: + application/json: + schema: + $ref: '#/components/schemas/StringResponse' + example: + error: true + code: repository_unreachable + message: 'Failed to fetch repository state: Git command failed: failed to resolve address for github: Name or service not known' '401': $ref: '#/components/responses/Unauthorized' '404': diff --git a/docs/src/tests.md b/docs/src/tests.md index e5575a29..e0a6a9dd 100644 --- a/docs/src/tests.md +++ b/docs/src/tests.md @@ -2658,3 +2658,33 @@ Run with: `pnpm --dir frontend exec ng test --watch=false` - `backend/core/src/db/admin_tasks.rs` — DB helper unit tests: insert/find/mark transitions, unique-violation detection, startup recovery `mark_all_active_failed`. - `backend/cache/src/cacher/deep_gc.rs` — sweep unit tests: blob pass removes orphan blob, blob pass purges zombie row, log pass removes orphan log, `DeepGcReport` serialises with snake_case keys. + +## Evaluation start - surface repository errors (issue #280) + +`POST /projects/{org}/{p}/evaluate` and `POST /projects/{org}/{p}/check-repository` +used to swallow git fetch failures (DNS, connection refused, auth) inside +`core::sources::git::check_for_updates` and bubble up a generic 500 with no +actionable detail. The fix propagates the `SourceError` to the web layer, which +maps it to `400 Bad Request` with `code: "repository_unreachable"` and the +underlying git error message. The frontend surfaces that message in an inline +banner under the project header instead of failing silently. + +### Backend + +Run with: `cargo test -p core --test git_remote` + +- `check_project_updates_propagates_unreachable_remote_error` — `git://127.0.0.1:1/…` + triggers an immediate connection-refused; the helper now returns `Err(SourceError)` + instead of `Ok((false, vec![]))`. Locks in the propagation guarantee that the + endpoint relies on for its 4xx mapping. + +### Frontend + +Run with: `pnpm --dir frontend exec ng test --watch=false` + +- `project-detail.component.spec.ts → 'shows an inline error banner when + startEvaluation fails'` — mocks `ProjectsService.startEvaluation` to throw; + asserts the `.evaluation-error` banner renders the underlying message. +- `project-detail.component.spec.ts → 'clears the error banner when the user + retries'` — calling `dismissError()` resets `errorMessage()` to `null` and the + banner disappears. diff --git a/frontend/src/app/features/projects/project-detail/project-detail.component.html b/frontend/src/app/features/projects/project-detail/project-detail.component.html index c25d39e8..ef24f0cf 100644 --- a/frontend/src/app/features/projects/project-detail/project-detail.component.html +++ b/frontend/src/app/features/projects/project-detail/project-detail.component.html @@ -57,6 +57,20 @@

{{ proj.display_name }}

[routerLink]="['/organization', orgName, 'project', projectName, 'metrics']" > + @if (errorMessage(); as msg) { + + }
link diff --git a/frontend/src/app/features/projects/project-detail/project-detail.component.scss b/frontend/src/app/features/projects/project-detail/project-detail.component.scss index 65fcc771..0d309527 100644 --- a/frontend/src/app/features/projects/project-detail/project-detail.component.scss +++ b/frontend/src/app/features/projects/project-detail/project-detail.component.scss @@ -92,6 +92,43 @@ } } +.evaluation-error { + flex-basis: 100%; + display: flex; + align-items: center; + gap: $spacing-sm; + padding: $spacing-sm $spacing-md; + background-color: rgba($color-danger, 0.1); + border: 1px solid $color-danger; + border-radius: $border-radius-sm; + color: $color-danger; + font-size: $font-size-sm; + + .material-symbols-outlined { + font-size: 18px; + flex-shrink: 0; + } + + .evaluation-error__text { + flex: 1 1 auto; + word-break: break-word; + } + + .evaluation-error__dismiss { + background: none; + border: 0; + color: inherit; + cursor: pointer; + padding: 0; + display: inline-flex; + align-items: center; + + .material-symbols-outlined { + font-size: 18px; + } + } +} + .stats-grid { display: grid; grid-template-columns: repeat(auto-fit, minmax(200px, 1fr)); diff --git a/frontend/src/app/features/projects/project-detail/project-detail.component.spec.ts b/frontend/src/app/features/projects/project-detail/project-detail.component.spec.ts index 097e3273..622a25f1 100644 --- a/frontend/src/app/features/projects/project-detail/project-detail.component.spec.ts +++ b/frontend/src/app/features/projects/project-detail/project-detail.component.spec.ts @@ -8,7 +8,7 @@ import { ComponentFixture, TestBed } from '@angular/core/testing'; import { ActivatedRoute, convertToParamMap, provideRouter } from '@angular/router'; import { provideHttpClient } from '@angular/common/http'; import { provideHttpClientTesting } from '@angular/common/http/testing'; -import { of } from 'rxjs'; +import { of, throwError } from 'rxjs'; import { ProjectDetailComponent } from './project-detail.component'; import { ProjectsService } from '@core/services/projects.service'; import { OrganizationsService } from '@core/services/organizations.service'; @@ -122,3 +122,57 @@ describe('ProjectDetailComponent - access gating', () => { expect(restartBtn!.disabled).toBe(false); }); }); + +// ── Issue #280: surface evaluation start failures to the user ──────────────── + +function setupWithStartError(message: string): ComponentFixture { + const access: AccessState = { managed: false, canEdit: true, canTrigger: true }; + TestBed.configureTestingModule({ + imports: [ProjectDetailComponent], + providers: [ + provideRouter([]), + provideHttpClient(), + provideHttpClientTesting(), + { provide: ActivatedRoute, useValue: activatedRouteStub(access) }, + { + provide: ProjectsService, + useValue: { + getProject: () => of(projectFor(access)), + getEntryPoints: () => of([]), + startEvaluation: () => throwError(() => new Error(message)), + restartFailedBuilds: () => throwError(() => new Error(message)), + }, + }, + { provide: OrganizationsService, useValue: { getOrganization: () => of({ display_name: 'Acme' }) } }, + { provide: AuthService, useValue: { isAuthenticated: () => true } }, + ], + }); + const fixture = TestBed.createComponent(ProjectDetailComponent); + fixture.detectChanges(); + return fixture; +} + +describe('ProjectDetailComponent - error surfacing (issue #280)', () => { + it('shows an inline error banner when startEvaluation fails', () => { + const fixture = setupWithStartError('Failed to fetch repository state: connection refused'); + fixture.componentInstance.startEvaluation(); + fixture.detectChanges(); + + const banner = fixture.nativeElement.querySelector('.evaluation-error') as HTMLElement | null; + expect(banner).not.toBeNull(); + expect(banner!.textContent).toContain('Failed to fetch repository state'); + }); + + it('clears the error banner when the user retries', () => { + const fixture = setupWithStartError('Failed to fetch repository state: connection refused'); + const component = fixture.componentInstance; + component.startEvaluation(); + fixture.detectChanges(); + expect(component.errorMessage()).not.toBeNull(); + + component.dismissError(); + fixture.detectChanges(); + expect(component.errorMessage()).toBeNull(); + expect(fixture.nativeElement.querySelector('.evaluation-error')).toBeNull(); + }); +}); diff --git a/frontend/src/app/features/projects/project-detail/project-detail.component.ts b/frontend/src/app/features/projects/project-detail/project-detail.component.ts index f166913a..0fb6469e 100644 --- a/frontend/src/app/features/projects/project-detail/project-detail.component.ts +++ b/frontend/src/app/features/projects/project-detail/project-detail.component.ts @@ -49,6 +49,7 @@ export class ProjectDetailComponent implements OnInit, OnDestroy { project = signal(null); entryPoints = signal([]); starting = signal(false); + errorMessage = signal(null); tick = signal(Date.now()); orgName = ''; @@ -120,6 +121,7 @@ export class ProjectDetailComponent implements OnInit, OnDestroy { startEvaluation(): void { this.starting.set(true); + this.errorMessage.set(null); this.projectsService.startEvaluation(this.orgName, this.projectName).subscribe({ next: () => { // Keep starting=true; polling will clear it once the evaluation appears @@ -127,6 +129,7 @@ export class ProjectDetailComponent implements OnInit, OnDestroy { }, error: (error) => { console.error('Failed to start evaluation:', error); + this.errorMessage.set(error?.message || 'Failed to start evaluation.'); this.starting.set(false); }, }); @@ -134,17 +137,23 @@ export class ProjectDetailComponent implements OnInit, OnDestroy { restartFailedBuilds(): void { this.starting.set(true); + this.errorMessage.set(null); this.projectsService.restartFailedBuilds(this.orgName, this.projectName).subscribe({ next: () => { this.loadProjectData(false); }, error: (error) => { console.error('Failed to restart failed builds:', error); + this.errorMessage.set(error?.message || 'Failed to restart failed builds.'); this.starting.set(false); }, }); } + dismissError(): void { + this.errorMessage.set(null); + } + abortEvaluation(evaluationId: string): void { this.projectsService.abortEvaluation(this.orgName, this.projectName, evaluationId).subscribe({ next: () => {