diff --git a/crates/common/src/models.rs b/crates/common/src/models.rs index da352c7..0a8a383 100644 --- a/crates/common/src/models.rs +++ b/crates/common/src/models.rs @@ -292,8 +292,6 @@ pub struct RemoteBuilder { pub created_at: DateTime, } -// --- User Management --- - /// User account for authentication and personalization #[derive(Debug, Clone, Serialize, Deserialize, FromRow)] pub struct User { @@ -352,7 +350,7 @@ pub struct UserSession { pub last_used_at: Option>, } -// --- Pagination --- +// Pagination #[derive(Debug, Clone, Serialize, Deserialize)] pub struct PaginationParams { @@ -389,7 +387,7 @@ pub struct PaginatedResponse { pub offset: i64, } -// --- DTO structs for creation and updates --- +// DTO structs for creation and updates #[derive(Debug, Clone, Serialize, Deserialize)] pub struct CreateProject { @@ -537,7 +535,7 @@ pub struct SystemStatus { pub channels_count: i64, } -// --- User DTOs --- +// User DTOs #[derive(Debug, Clone, Serialize, Deserialize)] pub struct CreateUser { diff --git a/crates/common/src/notifications.rs b/crates/common/src/notifications.rs index 45f6d25..207fba9 100644 --- a/crates/common/src/notifications.rs +++ b/crates/common/src/notifications.rs @@ -1,5 +1,7 @@ //! Notification dispatch for build events +use std::sync::OnceLock; + use tracing::{error, info, warn}; use crate::{ @@ -7,6 +9,13 @@ use crate::{ models::{Build, BuildStatus, Project}, }; +/// Shared HTTP client for all notification dispatches. +/// Avoids recreating connection pools on every build completion. +fn http_client() -> &'static reqwest::Client { + static CLIENT: OnceLock = OnceLock::new(); + CLIENT.get_or_init(reqwest::Client::new) +} + /// Dispatch all configured notifications for a completed build. pub async fn dispatch_build_finished( build: &Build, @@ -113,8 +122,7 @@ async fn set_github_status( "context": format!("fc/{}", build.job_name), }); - let client = reqwest::Client::new(); - match client + match http_client() .post(&url) .header("Authorization", format!("token {token}")) .header("User-Agent", "fc-ci") @@ -166,8 +174,7 @@ async fn set_gitea_status( "context": format!("fc/{}", build.job_name), }); - let client = reqwest::Client::new(); - match client + match http_client() .post(&url) .header("Authorization", format!("token {token}")) .json(&body) @@ -226,8 +233,7 @@ async fn set_gitlab_status( "name": format!("fc/{}", build.job_name), }); - let client = reqwest::Client::new(); - match client + match http_client() .post(&url) .header("PRIVATE-TOKEN", token) .json(&body) diff --git a/crates/common/tests/repo_tests.rs b/crates/common/tests/repo_tests.rs index 5303837..e41e045 100644 --- a/crates/common/tests/repo_tests.rs +++ b/crates/common/tests/repo_tests.rs @@ -93,7 +93,7 @@ async fn create_test_build( .expect("create build") } -// ---- Existing tests ---- +// CRUD and lifecycle tests #[tokio::test] async fn test_project_crud() { @@ -416,7 +416,7 @@ async fn test_not_found_errors() { )); } -// ---- New hardening tests ---- +// Batch operations and edge cases #[tokio::test] async fn test_batch_get_completed_by_drv_paths() { diff --git a/crates/evaluator/tests/eval_tests.rs b/crates/evaluator/tests/eval_tests.rs index 7c80d7d..30d4064 100644 --- a/crates/evaluator/tests/eval_tests.rs +++ b/crates/evaluator/tests/eval_tests.rs @@ -112,7 +112,7 @@ fn test_parse_nix_eval_jobs_both_attr_and_name() { assert_eq!(outputs.get("out").unwrap(), "/nix/store/abc123-hello"); } -// --- Inputs hash computation --- +// Inputs hash computation #[test] fn test_inputs_hash_deterministic() { diff --git a/crates/server/src/routes/badges.rs b/crates/server/src/routes/badges.rs index 18793ed..aded40e 100644 --- a/crates/server/src/routes/badges.rs +++ b/crates/server/src/routes/badges.rs @@ -137,17 +137,20 @@ async fn latest_build( } fn shield_svg(subject: &str, status: &str, color: &str) -> String { + use std::fmt::Write; + let subject_width = subject.len() * 7 + 10; let status_width = status.len() * 7 + 10; let total_width = subject_width + status_width; let subject_x = subject_width / 2; let status_x = subject_width + status_width / 2; - let mut svg = String::new(); - svg.push_str(&format!( + let mut svg = String::with_capacity(768); + let _ = writeln!( + svg, "\n" - )); + height=\"20\">" + ); svg.push_str(" \n"); svg.push_str( " \n", @@ -155,41 +158,43 @@ fn shield_svg(subject: &str, status: &str, color: &str) -> String { svg.push_str(" \n"); svg.push_str(" \n"); svg.push_str(" \n"); - svg.push_str(&format!( - " \n" - )); + let _ = writeln!( + svg, + " " + ); svg.push_str(" \n"); svg.push_str(" \n"); - svg.push_str(&format!( - " \n" - )); - svg.push_str(&format!( + let _ = writeln!( + svg, + " " + ); + let _ = writeln!( + svg, " \n" - )); - svg.push_str(&format!( - " \n" - )); + fill=\"{color}\"/>" + ); + let _ = writeln!( + svg, + " " + ); svg.push_str(" \n"); svg.push_str( " \n", ); - svg.push_str(&format!( + let _ = writeln!( + svg, " {subject}\n" - )); - svg.push_str(&format!( - " {subject}\n" - )); - svg.push_str(&format!( + fill-opacity=\".3\">{subject}" + ); + let _ = + writeln!(svg, " {subject}"); + let _ = writeln!( + svg, " {status}\n" - )); - svg.push_str(&format!( - " {status}\n" - )); + fill-opacity=\".3\">{status}" + ); + let _ = writeln!(svg, " {status}"); svg.push_str(" \n"); svg.push_str(""); svg @@ -200,3 +205,50 @@ pub fn router() -> Router { .route("/job/{project}/{jobset}/{job}/shield", get(build_badge)) .route("/job/{project}/{jobset}/{job}/latest", get(latest_build)) } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_shield_svg_is_valid_svg() { + let svg = shield_svg("build", "passing", "#4c1"); + assert!(svg.starts_with("")); + assert!(svg.contains("build")); + assert!(svg.contains("passing")); + assert!(svg.contains("#4c1")); + } + + #[test] + fn test_shield_svg_dimensions() { + let svg = shield_svg("build", "failing", "#e05d44"); + // "build" = 5 chars * 7 + 10 = 45 + // "failing" = 7 chars * 7 + 10 = 59 + // total = 104 + assert!(svg.contains("width=\"104\"")); + } + + #[test] + fn test_shield_svg_text_positions() { + let svg = shield_svg("ci", "ok", "#4c1"); + // "ci" = 2*7+10 = 24, subject_x = 12 + // "ok" = 2*7+10 = 24, status_x = 24 + 12 = 36 + assert!(svg.contains("x=\"12\"")); + assert!(svg.contains("x=\"36\"")); + } + + #[test] + fn test_shield_svg_different_statuses() { + for (status, color) in [ + ("passing", "#4c1"), + ("failing", "#e05d44"), + ("building", "#dfb317"), + ("not found", "#9f9f9f"), + ] { + let svg = shield_svg("build", status, color); + assert!(svg.contains(status), "SVG should contain status '{status}'"); + assert!(svg.contains(color), "SVG should contain color '{color}'"); + } + } +} diff --git a/crates/server/src/routes/jobsets.rs b/crates/server/src/routes/jobsets.rs index a07a16a..c4dbb10 100644 --- a/crates/server/src/routes/jobsets.rs +++ b/crates/server/src/routes/jobsets.rs @@ -46,7 +46,7 @@ async fn delete_jobset( Ok(Json(serde_json::json!({ "deleted": true }))) } -// --- Jobset input routes --- +// Jobset input routes async fn list_jobset_inputs( State(state): State, diff --git a/crates/server/src/routes/metrics.rs b/crates/server/src/routes/metrics.rs index 2407a13..740db5e 100644 --- a/crates/server/src/routes/metrics.rs +++ b/crates/server/src/routes/metrics.rs @@ -124,31 +124,38 @@ async fn prometheus_metrics(State(state): State) -> Response { .await .unwrap_or((None, None, None)); - let mut output = String::new(); + use std::fmt::Write; + + let mut output = String::with_capacity(2048); // Build counts by status output.push_str("# HELP fc_builds_total Total number of builds by status\n"); output.push_str("# TYPE fc_builds_total gauge\n"); - output.push_str(&format!( - "fc_builds_total{{status=\"completed\"}} {}\n", + let _ = writeln!( + output, + "fc_builds_total{{status=\"completed\"}} {}", stats.completed_builds.unwrap_or(0) - )); - output.push_str(&format!( - "fc_builds_total{{status=\"failed\"}} {}\n", + ); + let _ = writeln!( + output, + "fc_builds_total{{status=\"failed\"}} {}", stats.failed_builds.unwrap_or(0) - )); - output.push_str(&format!( - "fc_builds_total{{status=\"running\"}} {}\n", + ); + let _ = writeln!( + output, + "fc_builds_total{{status=\"running\"}} {}", stats.running_builds.unwrap_or(0) - )); - output.push_str(&format!( - "fc_builds_total{{status=\"pending\"}} {}\n", + ); + let _ = writeln!( + output, + "fc_builds_total{{status=\"pending\"}} {}", stats.pending_builds.unwrap_or(0) - )); - output.push_str(&format!( - "fc_builds_total{{status=\"all\"}} {}\n", + ); + let _ = writeln!( + output, + "fc_builds_total{{status=\"all\"}} {}", stats.total_builds.unwrap_or(0) - )); + ); // Build duration stats output.push_str( @@ -156,67 +163,73 @@ async fn prometheus_metrics(State(state): State) -> Response { seconds\n", ); output.push_str("# TYPE fc_builds_avg_duration_seconds gauge\n"); - output.push_str(&format!( - "fc_builds_avg_duration_seconds {:.2}\n", + let _ = writeln!( + output, + "fc_builds_avg_duration_seconds {:.2}", stats.avg_duration_seconds.unwrap_or(0.0) - )); + ); output.push_str( "\n# HELP fc_builds_duration_seconds Build duration percentiles\n", ); output.push_str("# TYPE fc_builds_duration_seconds gauge\n"); if let Some(p50) = duration_p50 { - output.push_str(&format!( - "fc_builds_duration_seconds{{quantile=\"0.5\"}} {p50:.2}\n" - )); + let _ = writeln!( + output, + "fc_builds_duration_seconds{{quantile=\"0.5\"}} {p50:.2}" + ); } if let Some(p95) = duration_p95 { - output.push_str(&format!( - "fc_builds_duration_seconds{{quantile=\"0.95\"}} {p95:.2}\n" - )); + let _ = writeln!( + output, + "fc_builds_duration_seconds{{quantile=\"0.95\"}} {p95:.2}" + ); } if let Some(p99) = duration_p99 { - output.push_str(&format!( - "fc_builds_duration_seconds{{quantile=\"0.99\"}} {p99:.2}\n" - )); + let _ = writeln!( + output, + "fc_builds_duration_seconds{{quantile=\"0.99\"}} {p99:.2}" + ); } // Evaluations output .push_str("\n# HELP fc_evaluations_total Total number of evaluations\n"); output.push_str("# TYPE fc_evaluations_total gauge\n"); - output.push_str(&format!("fc_evaluations_total {eval_count}\n")); + let _ = writeln!(output, "fc_evaluations_total {eval_count}"); output.push_str("\n# HELP fc_evaluations_by_status Evaluations by status\n"); output.push_str("# TYPE fc_evaluations_by_status gauge\n"); for (status, count) in &eval_by_status { - output.push_str(&format!( - "fc_evaluations_by_status{{status=\"{status}\"}} {count}\n" - )); + let _ = writeln!( + output, + "fc_evaluations_by_status{{status=\"{status}\"}} {count}" + ); } // Queue depth (pending builds) output .push_str("\n# HELP fc_queue_depth Number of pending builds in queue\n"); output.push_str("# TYPE fc_queue_depth gauge\n"); - output.push_str(&format!( - "fc_queue_depth {}\n", + let _ = writeln!( + output, + "fc_queue_depth {}", stats.pending_builds.unwrap_or(0) - )); + ); // Infrastructure output.push_str("\n# HELP fc_projects_total Total number of projects\n"); output.push_str("# TYPE fc_projects_total gauge\n"); - output.push_str(&format!("fc_projects_total {project_count}\n")); + let _ = writeln!(output, "fc_projects_total {project_count}"); output.push_str("\n# HELP fc_channels_total Total number of channels\n"); output.push_str("# TYPE fc_channels_total gauge\n"); - output.push_str(&format!("fc_channels_total {channel_count}\n")); + let _ = writeln!(output, "fc_channels_total {channel_count}"); output .push_str("\n# HELP fc_remote_builders_active Active remote builders\n"); output.push_str("# TYPE fc_remote_builders_active gauge\n"); - output.push_str(&format!("fc_remote_builders_active {builder_count}\n")); + let _ = writeln!(output, "fc_remote_builders_active {builder_count}"); // Per-project build counts if !per_project.is_empty() { @@ -226,9 +239,10 @@ async fn prometheus_metrics(State(state): State) -> Response { output.push_str("# TYPE fc_project_builds_completed gauge\n"); for (name, completed, _) in &per_project { let escaped = escape_prometheus_label(name); - output.push_str(&format!( - "fc_project_builds_completed{{project=\"{escaped}\"}} {completed}\n" - )); + let _ = writeln!( + output, + "fc_project_builds_completed{{project=\"{escaped}\"}} {completed}" + ); } output.push_str( "\n# HELP fc_project_builds_failed Failed builds per project\n", @@ -236,9 +250,10 @@ async fn prometheus_metrics(State(state): State) -> Response { output.push_str("# TYPE fc_project_builds_failed gauge\n"); for (name, _, failed) in &per_project { let escaped = escape_prometheus_label(name); - output.push_str(&format!( - "fc_project_builds_failed{{project=\"{escaped}\"}} {failed}\n" - )); + let _ = writeln!( + output, + "fc_project_builds_failed{{project=\"{escaped}\"}} {failed}" + ); } } @@ -354,3 +369,33 @@ pub fn router() -> Router { ) .route("/api/v1/metrics/systems", get(system_distribution)) } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_escape_prometheus_label_plain() { + assert_eq!(escape_prometheus_label("hello"), "hello"); + } + + #[test] + fn test_escape_prometheus_label_backslash() { + assert_eq!(escape_prometheus_label(r"a\b"), r"a\\b"); + } + + #[test] + fn test_escape_prometheus_label_quotes() { + assert_eq!(escape_prometheus_label(r#"say "hi""#), r#"say \"hi\""#); + } + + #[test] + fn test_escape_prometheus_label_newline() { + assert_eq!(escape_prometheus_label("line1\nline2"), r"line1\nline2"); + } + + #[test] + fn test_escape_prometheus_label_combined() { + assert_eq!(escape_prometheus_label("a\\b\n\"c\""), r#"a\\b\n\"c\""#); + } +} diff --git a/crates/server/src/routes/users.rs b/crates/server/src/routes/users.rs index e41a843..a9b2642 100644 --- a/crates/server/src/routes/users.rs +++ b/crates/server/src/routes/users.rs @@ -16,7 +16,7 @@ use uuid::Uuid; use crate::{auth_middleware::RequireAdmin, error::ApiError, state::AppState}; -// --- DTOs --- +// Request/response DTOs #[derive(Debug, Deserialize)] pub struct CreateUserRequest { @@ -92,7 +92,7 @@ pub struct StarredJobResponse { pub created_at: chrono::DateTime, } -// --- Handlers --- +// Admin user management handlers async fn list_users( _auth: RequireAdmin, @@ -165,7 +165,7 @@ async fn delete_user( Ok(StatusCode::NO_CONTENT) } -// --- Current User Handlers --- +// Current user (self-service) handlers async fn get_current_user( extensions: axum::http::Extensions, @@ -283,7 +283,7 @@ pub struct ChangePasswordRequest { pub new_password: String, } -// --- Starred Jobs Handlers --- +// Starred jobs handlers async fn list_starred_jobs( State(state): State, diff --git a/crates/server/tests/api_tests.rs b/crates/server/tests/api_tests.rs index bebb71f..7aa9d27 100644 --- a/crates/server/tests/api_tests.rs +++ b/crates/server/tests/api_tests.rs @@ -75,7 +75,7 @@ fn build_app_with_config( fc_server::routes::router(state, &server_config) } -// ---- Existing tests ---- +// API endpoint tests #[tokio::test] async fn test_health_endpoint() { @@ -240,7 +240,7 @@ async fn test_builds_endpoints() { assert_eq!(response.status(), StatusCode::OK); } -// ---- Hardening tests ---- +// Error response structure #[tokio::test] async fn test_error_response_includes_error_code() { @@ -663,9 +663,57 @@ async fn test_metrics_endpoint() { .await .unwrap(); let body_str = String::from_utf8(body.to_vec()).unwrap(); + + // Verify metric names are present assert!(body_str.contains("fc_builds_total")); assert!(body_str.contains("fc_projects_total")); assert!(body_str.contains("fc_evaluations_total")); + + // Verify Prometheus format: HELP/TYPE headers and label syntax + assert!( + body_str.contains("# HELP fc_builds_total"), + "Missing HELP header for fc_builds_total" + ); + assert!( + body_str.contains("# TYPE fc_builds_total gauge"), + "Missing TYPE header for fc_builds_total" + ); + assert!( + body_str.contains("fc_builds_total{status=\"completed\"}"), + "Missing completed status label" + ); + assert!( + body_str.contains("fc_builds_total{status=\"failed\"}"), + "Missing failed status label" + ); + assert!( + body_str.contains("fc_queue_depth"), + "Missing queue depth metric" + ); + assert!( + body_str.contains("fc_builds_avg_duration_seconds"), + "Missing avg duration metric" + ); + + // Verify each line with a metric value ends with a number (basic format + // check) + for line in body_str.lines() { + if line.starts_with('#') || line.is_empty() { + continue; + } + // Metric lines should have format: metric_name{labels} value + // or: metric_name value + let parts: Vec<&str> = line.rsplitn(2, ' ').collect(); + assert!( + parts.len() == 2, + "Malformed metric line (expected 'name value'): {line}" + ); + assert!( + parts[0].parse::().is_ok(), + "Metric value is not a number: '{}' in line: {line}", + parts[0] + ); + } } #[tokio::test] @@ -698,7 +746,7 @@ async fn test_get_nonexistent_build_returns_error_code() { assert!(json["error"].as_str().unwrap().contains("not found")); } -// ---- Validation tests ---- +// Input validation #[tokio::test] async fn test_create_project_validation_rejects_invalid_name() { @@ -802,7 +850,7 @@ async fn test_create_project_validation_accepts_valid() { assert_eq!(response.status(), StatusCode::OK); } -// ---- Error handling tests ---- +// Auth and error handling #[tokio::test] async fn test_project_create_with_auth() {