Propagate web handler errors instead of swallowing them
Assisted-by: Claude Sonnet 4.6 via Claude Code
diff --git a/quire-server/src/bin/quire/commands/exec.rs b/quire-server/src/bin/quire/commands/exec.rs
index dff91a7..0745d5b 100644
--- a/quire-server/src/bin/quire/commands/exec.rs
+++ b/quire-server/src/bin/quire/commands/exec.rs
@@ -45,7 +45,6 @@ fn dispatch_git(quire: &Quire, git_cmd: &str, args: &[String]) -> Result<()> {
ensure!(!path.is_empty(), "empty repository path");
let repo = quire.repo(path)?;
- ensure!(repo.exists(), "repository not found: {path}");
tracing::info!(%git_cmd, %path, "dispatching git command");
// Use `git <subcommand>` instead of `git-<subcommand>` so the git
diff --git a/quire-server/src/bin/quire/commands/repo.rs b/quire-server/src/bin/quire/commands/repo.rs
index 9b98f22..669141c 100644
--- a/quire-server/src/bin/quire/commands/repo.rs
+++ b/quire-server/src/bin/quire/commands/repo.rs
@@ -3,9 +3,10 @@ use std::process::Command;
use miette::{IntoDiagnostic, Result, ensure};
use quire::Quire;
+use quire::quire::Repo;
pub async fn new(quire: &Quire, name: &str) -> Result<()> {
- let repo = quire.repo(name)?;
+ let repo = Repo::new(&quire.repos_dir(), name)?;
ensure!(!repo.exists(), "repository already exists: {name}");
// Create parent directory for grouped repos (e.g. work/foo.git).
@@ -36,7 +37,6 @@ pub async fn list(quire: &Quire) -> Result<()> {
pub async fn rm(quire: &Quire, name: &str) -> Result<()> {
let repo = quire.repo(name)?;
- ensure!(repo.exists(), "repository not found: {name}");
fs_err::remove_dir_all(repo.path()).into_diagnostic()?;
diff --git a/quire-server/src/ci/mod.rs b/quire-server/src/ci/mod.rs
index 582bbe5..1560d61 100644
--- a/quire-server/src/ci/mod.rs
+++ b/quire-server/src/ci/mod.rs
@@ -116,13 +116,9 @@ struct RunContext<'a> {
/// create a run, and evaluate + validate it.
pub fn trigger(quire: &crate::Quire, event: &PushEvent) {
let repo = match quire.repo(&event.repo) {
- Ok(r) if r.exists() => r,
- Ok(_) => {
- tracing::error!(repo = %event.repo, "repo not found on disk");
- return;
- }
+ Ok(r) => r,
Err(e) => {
- tracing::error!(repo = %event.repo, error = %e, "invalid repo name in event");
+ tracing::error!(repo = %event.repo, error = %e, "failed to resolve repo");
return;
}
};
diff --git a/quire-server/src/error.rs b/quire-server/src/error.rs
index c48adb5..c5fbbf2 100644
--- a/quire-server/src/error.rs
+++ b/quire-server/src/error.rs
@@ -12,6 +12,9 @@ pub enum Error {
#[error(transparent)]
Repo(#[from] RepoNameError),
+ #[error("repository not found: {0}")]
+ RepoNotFound(String),
+
#[error(transparent)]
#[diagnostic(transparent)]
Fennel(#[from] Box<FennelError>),
diff --git a/quire-server/src/mirror.rs b/quire-server/src/mirror.rs
index 0cedf6c..c369783 100644
--- a/quire-server/src/mirror.rs
+++ b/quire-server/src/mirror.rs
@@ -17,9 +17,6 @@ pub struct MirrorErrors {
#[derive(Debug, Error, Diagnostic)]
enum MirrorError {
- #[error("repo not found on disk: {0}")]
- RepoNotFound(String),
-
#[error("git push to {url} failed: {stderr}")]
PushFailed { url: String, stderr: String },
@@ -52,10 +49,6 @@ fn collect_errors(quire: &Quire, event: &PushEvent) -> Result<Vec<MirrorError>,
.repo(&event.repo)
.map_err(|e| one(MirrorError::App(e)))?;
- if !repo.exists() {
- return Err(one(MirrorError::RepoNotFound(event.repo.clone())));
- }
-
let config = &quire.config;
let Some(mirror_token) = config
.github
diff --git a/quire-server/src/quire/mod.rs b/quire-server/src/quire/mod.rs
index 2333701..85bec74 100644
--- a/quire-server/src/quire/mod.rs
+++ b/quire-server/src/quire/mod.rs
@@ -278,11 +278,17 @@ impl Quire {
mutex.lock().unwrap_or_else(|e| e.into_inner())
}
- /// Validate a repository name and return its resolved path.
+ /// Look up an existing repository by name.
///
- /// Delegates to `Repo::new` for name validation.
+ /// Returns an error if the name fails validation or the repo does not
+ /// exist on disk.
pub fn repo(&self, name: &str) -> Result<Repo> {
- Repo::new(&self.repos_dir(), name)
+ let repo = Repo::new(&self.repos_dir(), name)?;
+ if repo.exists() {
+ Ok(repo)
+ } else {
+ Err(crate::Error::RepoNotFound(name.to_string()))
+ }
}
/// Resolve a filesystem path to a `Repo`.
@@ -352,14 +358,14 @@ mod tests {
#[test]
fn repo_valid() {
let q = quire();
- assert!(q.repo("foo.git").is_ok());
- assert!(q.repo("work/foo.git").is_ok());
+ assert!(Repo::new(&q.repos_dir(), "foo.git").is_ok());
+ assert!(Repo::new(&q.repos_dir(), "work/foo.git").is_ok());
}
#[test]
fn repo_name_returns_name() {
let q = quire();
- let repo = q.repo("foo.git").unwrap();
+ let repo = Repo::new(&q.repos_dir(), "foo.git").unwrap();
assert_eq!(repo.name(), "foo.git");
}
@@ -394,7 +400,7 @@ mod tests {
fn repo_resolves_path() {
let q = quire();
assert_eq!(
- q.repo("foo.git").unwrap().path(),
+ Repo::new(&q.repos_dir(), "foo.git").unwrap().path(),
Path::new("/var/quire/repos/foo.git")
);
}
diff --git a/quire-server/src/quire/web/error.rs b/quire-server/src/quire/web/error.rs
new file mode 100644
index 0000000..768be89
--- /dev/null
+++ b/quire-server/src/quire/web/error.rs
@@ -0,0 +1,28 @@
+//! Web handler error type.
+
+use axum::http::StatusCode;
+use axum::response::{IntoResponse, Response};
+
+#[derive(Debug, thiserror::Error)]
+pub enum WebError {
+ #[error(transparent)]
+ Internal(#[from] crate::Error),
+
+ #[error(transparent)]
+ TaskPanic(#[from] tokio::task::JoinError),
+}
+
+impl IntoResponse for WebError {
+ fn into_response(self) -> Response {
+ match self {
+ Self::Internal(
+ crate::Error::RepoNotFound(_)
+ | crate::Error::Sql(rusqlite::Error::QueryReturnedNoRows),
+ ) => StatusCode::NOT_FOUND.into_response(),
+ Self::Internal(_) | Self::TaskPanic(_) => {
+ StatusCode::INTERNAL_SERVER_ERROR.into_response()
+ }
+ }
+ }
+}
+
diff --git a/quire-server/src/quire/web/handlers/ci.rs b/quire-server/src/quire/web/handlers/ci.rs
index 33c7c23..7d1080c 100644
--- a/quire-server/src/quire/web/handlers/ci.rs
+++ b/quire-server/src/quire/web/handlers/ci.rs
@@ -1,46 +1,29 @@
//! Handlers for CI run list and run detail pages.
use axum::extract::State;
-use axum::http::StatusCode;
-use axum::response::{IntoResponse, Response};
+use axum::response::Response;
use super::super::db;
+use super::super::error::WebError;
use super::super::templates::{
DetailJob, DetailRun, DetailShEvent, RunDetailTemplate, RunListRow, RunListTemplate,
nav_sections,
};
-use super::{render, render_error};
+use super::render;
use crate::Quire;
use crate::quire::web::paths::{RunDetailPath, RunListPath};
-pub async fn run_list(RunListPath { repo }: RunListPath, State(quire): State<Quire>) -> Response {
+pub async fn run_list(
+ RunListPath { repo }: RunListPath,
+ State(quire): State<Quire>,
+) -> Result<Response, WebError> {
let repo_display = repo.trim_end_matches(".git").to_string();
let repo_name = db::resolve_repo_name(&repo);
- match quire.repo(&repo_name) {
- Ok(r) if r.exists() => {}
- _ => return StatusCode::NOT_FOUND.into_response(),
- };
+ quire.repo(&repo_name)?;
let q = quire.clone();
let rn = repo_name.clone();
- let runs_handle = tokio::task::spawn_blocking(move || db::load_runs(&q, &rn));
-
- let runs = match runs_handle.await {
- Ok(Ok(r)) => r,
- Ok(Err(e)) => {
- tracing::error!(repo = %repo, error = &e as &(dyn std::error::Error + 'static), "failed to load runs");
- return render_error(
- repo_display,
- StatusCode::INTERNAL_SERVER_ERROR,
- "Failed to load runs",
- e.to_string(),
- );
- }
- Err(_) => {
- tracing::error!("spawn_blocking task panicked");
- return StatusCode::INTERNAL_SERVER_ERROR.into_response();
- }
- };
+ let runs = tokio::task::spawn_blocking(move || db::load_runs(&q, &rn)).await??;
let template_runs: Vec<RunListRow> = runs
.into_iter()
@@ -61,44 +44,21 @@ pub async fn run_list(RunListPath { repo }: RunListPath, State(quire): State<Qui
crumbs: None,
runs: template_runs,
};
- render(&tmpl)
+ Ok(render(&tmpl))
}
pub async fn run_detail(
RunDetailPath { repo, run_id }: RunDetailPath,
State(quire): State<Quire>,
-) -> Response {
+) -> Result<Response, WebError> {
let repo_display = repo.trim_end_matches(".git").to_string();
let repo_name = db::resolve_repo_name(&repo);
- match quire.repo(&repo_name) {
- Ok(r) if r.exists() => {}
- _ => return StatusCode::NOT_FOUND.into_response(),
- };
- if !db::is_valid_run_id(&run_id) {
- return StatusCode::NOT_FOUND.into_response();
- }
+ quire.repo(&repo_name)?;
let q = quire.clone();
let rn = repo_name.clone();
let ri = run_id.clone();
- let detail = match tokio::task::spawn_blocking(move || db::load_run_detail(&q, &rn, &ri)).await
- {
- Ok(Ok(d)) => d,
- Ok(Err(ref e)) if is_no_rows(e) => return StatusCode::NOT_FOUND.into_response(),
- Ok(Err(e)) => {
- tracing::error!(repo = %repo, run_id = %run_id, error = &e as &(dyn std::error::Error + 'static), "failed to load run detail");
- return render_error(
- repo_display,
- StatusCode::INTERNAL_SERVER_ERROR,
- "Failed to load run",
- e.to_string(),
- );
- }
- Err(_) => {
- tracing::error!("spawn_blocking task panicked");
- return StatusCode::INTERNAL_SERVER_ERROR.into_response();
- }
- };
+ let detail = tokio::task::spawn_blocking(move || db::load_run_detail(&q, &rn, &ri)).await??;
let detail_run = DetailRun {
outcome: detail.run.outcome,
@@ -201,14 +161,7 @@ pub async fn run_detail(
jobs: detail_jobs,
quire_ci_log,
};
- render(&tmpl)
-}
-
-fn is_no_rows(err: &crate::error::Error) -> bool {
- matches!(
- err,
- crate::error::Error::Sql(rusqlite::Error::QueryReturnedNoRows)
- )
+ Ok(render(&tmpl))
}
async fn read_log(path: &std::path::Path) -> String {
diff --git a/quire-server/src/quire/web/handlers/commit.rs b/quire-server/src/quire/web/handlers/commit.rs
index b340b4b..9b10009 100644
--- a/quire-server/src/quire/web/handlers/commit.rs
+++ b/quire-server/src/quire/web/handlers/commit.rs
@@ -2,9 +2,9 @@
use axum::extract::State;
use axum::http::StatusCode;
-use axum::response::IntoResponse;
-use axum::response::Response;
+use axum::response::{IntoResponse, Response};
+use super::super::error::WebError;
use super::super::templates::{CommitParent, CommitTemplate, nav_sections};
use super::git::RepoView;
use super::render;
@@ -15,13 +15,10 @@ pub async fn commit_view(
CommitPath { repo, sha }: CommitPath,
State(quire): State<Quire>,
auth: super::super::auth::Auth,
-) -> Response {
+) -> Result<Response, WebError> {
let repo_display = repo.trim_end_matches(".git").to_string();
let repo_name = super::super::db::resolve_repo_name(&repo);
- let git_repo = match quire.repo(&repo_name) {
- Ok(r) if r.exists() => r,
- _ => return StatusCode::NOT_FOUND.into_response(),
- };
+ let git_repo = quire.repo(&repo_name)?;
let sha_clone = sha.clone();
let result = tokio::task::spawn_blocking(move || {
@@ -82,12 +79,10 @@ pub async fn commit_view(
diff,
))
})
- .await
- .unwrap_or(None);
+ .await?;
- let (sha, author, email, timestamp_ms, subject, body, parent_shas, diff) = match result {
- Some(data) => data,
- None => return StatusCode::NOT_FOUND.into_response(),
+ let Some((sha, author, email, timestamp_ms, subject, body, parent_shas, diff)) = result else {
+ return Ok(StatusCode::NOT_FOUND.into_response());
};
let parents: Vec<CommitParent> = parent_shas
@@ -125,5 +120,5 @@ pub async fn commit_view(
parents,
diff,
};
- render(&tmpl)
+ Ok(render(&tmpl))
}
diff --git a/quire-server/src/quire/web/handlers/log_view.rs b/quire-server/src/quire/web/handlers/log_view.rs
index a34eecb..09f2d7d 100644
--- a/quire-server/src/quire/web/handlers/log_view.rs
+++ b/quire-server/src/quire/web/handlers/log_view.rs
@@ -1,10 +1,9 @@
//! Handler for the repository commit log page.
use axum::extract::State;
-use axum::http::StatusCode;
-use axum::response::IntoResponse;
use axum::response::Response;
+use super::super::error::WebError;
use super::super::templates::{LogTemplate, nav_sections};
use super::git::RepoView;
use super::render;
@@ -15,13 +14,10 @@ pub async fn log_view(
LogPath { repo }: LogPath,
State(quire): State<Quire>,
auth: super::super::auth::Auth,
-) -> Response {
+) -> Result<Response, WebError> {
let repo_display = repo.trim_end_matches(".git").to_string();
let repo_name = super::super::db::resolve_repo_name(&repo);
- let git_repo = match quire.repo(&repo_name) {
- Ok(r) if r.exists() => r,
- _ => return StatusCode::NOT_FOUND.into_response(),
- };
+ let git_repo = quire.repo(&repo_name)?;
let repo_d = repo_display.clone();
let (changes, bookmark, sha_short) = tokio::task::spawn_blocking(move || {
@@ -35,8 +31,7 @@ pub async fn log_view(
.unwrap_or_else(|| "unknown".to_string());
(changes, bookmark, sha_short)
})
- .await
- .unwrap_or_default();
+ .await?;
let tmpl = LogTemplate {
sections: nav_sections(&repo_display, "log", auth.is_authenticated()),
@@ -46,5 +41,5 @@ pub async fn log_view(
bookmark,
sha_short,
};
- render(&tmpl)
+ Ok(render(&tmpl))
}
diff --git a/quire-server/src/quire/web/handlers/mod.rs b/quire-server/src/quire/web/handlers/mod.rs
index 0484640..7b83a90 100644
--- a/quire-server/src/quire/web/handlers/mod.rs
+++ b/quire-server/src/quire/web/handlers/mod.rs
@@ -20,7 +20,7 @@ use axum::extract::State;
use axum::http::{StatusCode, header};
use axum::response::{Html, IntoResponse, Response};
-use super::templates::{ConfigTemplate, ErrorTemplate};
+use super::templates::ConfigTemplate;
use crate::Quire;
/// Render a template into an HTML response, returning 500 on render failure.
@@ -37,32 +37,6 @@ pub(super) fn render<T: Template>(tmpl: &T) -> Response {
}
}
-/// Render the error template with the given status, falling back to plain
-/// text if the error template itself fails to render.
-pub(super) fn render_error(
- repo: String,
- status: StatusCode,
- title: &str,
- detail: String,
-) -> Response {
- let tmpl = ErrorTemplate {
- repo,
- crumbs: None,
- title: title.to_string(),
- detail: detail.clone(),
- };
- match tmpl.render() {
- Ok(body) => (status, Html(body)).into_response(),
- Err(e) => {
- tracing::error!(
- error = &e as &(dyn std::error::Error + 'static),
- "error template render failed"
- );
- (status, format!("{title}\n\n{detail}\n")).into_response()
- }
- }
-}
-
/// Serve the compiled-in stylesheet.
pub async fn stylesheet() -> Response {
(
diff --git a/quire-server/src/quire/web/handlers/repo.rs b/quire-server/src/quire/web/handlers/repo.rs
index ba365c9..fa603b0 100644
--- a/quire-server/src/quire/web/handlers/repo.rs
+++ b/quire-server/src/quire/web/handlers/repo.rs
@@ -1,12 +1,11 @@
//! Handler for the repository home page.
use axum::extract::State;
-use axum::http::StatusCode;
-use axum::response::IntoResponse;
use axum::response::Response;
use super::super::auth::Auth;
use super::super::db;
+use super::super::error::WebError;
use super::super::templates::{RepoHomeTemplate, RunListRow, nav_sections};
use super::git::RepoView;
use super::render;
@@ -17,47 +16,36 @@ pub async fn repo_home(
RepoPath { repo }: RepoPath,
State(quire): State<Quire>,
auth: Auth,
-) -> Response {
+) -> Result<Response, WebError> {
let repo_display = repo.trim_end_matches(".git").to_string();
let repo_name = db::resolve_repo_name(&repo);
- let git_repo = match quire.repo(&repo_name) {
- Ok(r) if r.exists() => r,
- _ => return StatusCode::NOT_FOUND.into_response(),
- };
+ let git_repo = quire.repo(&repo_name)?;
let q = quire.clone();
let rn = repo_name.clone();
let is_authed = auth.is_authenticated();
let recent_runs: Vec<RunListRow> = if is_authed {
- match tokio::task::spawn_blocking(move || db::load_runs(&q, &rn)).await {
- Ok(Ok(runs)) => runs
- .into_iter()
- .take(5)
- .map(|r| RunListRow {
- id: r.id,
- outcome: r.outcome,
- sha: r.sha,
- ref_name: r.ref_name,
- created_at: r.created_at,
- dispatched_at: r.dispatched_at,
- resolved_at: r.resolved_at,
- })
- .collect(),
- Ok(Err(e)) => {
- tracing::warn!(repo = %repo, error = &e as &(dyn std::error::Error + 'static), "failed to load runs for home");
- vec![]
- }
- Err(_) => vec![],
- }
+ tokio::task::spawn_blocking(move || db::load_runs(&q, &rn))
+ .await??
+ .into_iter()
+ .take(5)
+ .map(|r| RunListRow {
+ id: r.id,
+ outcome: r.outcome,
+ sha: r.sha,
+ ref_name: r.ref_name,
+ created_at: r.created_at,
+ dispatched_at: r.dispatched_at,
+ resolved_at: r.resolved_at,
+ })
+ .collect()
} else {
vec![]
};
let rd = repo_display.clone();
let (head, readme_html, recent_changes) =
- tokio::task::spawn_blocking(move || RepoView::new(&git_repo).read_all(&rd))
- .await
- .unwrap_or_default();
+ tokio::task::spawn_blocking(move || RepoView::new(&git_repo).read_all(&rd)).await?;
let tmpl = RepoHomeTemplate {
sections: nav_sections(&repo_display, "overview", is_authed),
@@ -68,5 +56,5 @@ pub async fn repo_home(
recent_runs,
recent_changes,
};
- render(&tmpl)
+ Ok(render(&tmpl))
}
diff --git a/quire-server/src/quire/web/handlers/tree.rs b/quire-server/src/quire/web/handlers/tree.rs
index 82be2d2..055d587 100644
--- a/quire-server/src/quire/web/handlers/tree.rs
+++ b/quire-server/src/quire/web/handlers/tree.rs
@@ -6,6 +6,7 @@ use axum::response::{IntoResponse, Response};
use super::super::auth::Auth;
use super::super::db;
+use super::super::error::WebError;
use super::super::templates::{
Crumb, FileViewTemplate, TreeEntry, TreeEntryKind, TreeTemplate, nav_sections,
};
@@ -18,7 +19,7 @@ pub async fn tree_view(
TreeRootPath { repo }: TreeRootPath,
State(quire): State<Quire>,
auth: Auth,
-) -> Response {
+) -> Result<Response, WebError> {
tree_or_file_at_path(quire, repo, String::new(), auth.is_authenticated()).await
}
@@ -26,17 +27,19 @@ pub async fn tree_view_path(
TreePath { repo, path }: TreePath,
State(quire): State<Quire>,
auth: Auth,
-) -> Response {
+) -> Result<Response, WebError> {
tree_or_file_at_path(quire, repo, path, auth.is_authenticated()).await
}
-async fn tree_or_file_at_path(quire: Quire, repo: String, path: String, authed: bool) -> Response {
+async fn tree_or_file_at_path(
+ quire: Quire,
+ repo: String,
+ path: String,
+ authed: bool,
+) -> Result<Response, WebError> {
let repo_display = repo.trim_end_matches(".git").to_string();
let repo_name = db::resolve_repo_name(&repo);
- let git_repo = match quire.repo(&repo_name) {
- Ok(r) if r.exists() => r,
- _ => return StatusCode::NOT_FOUND.into_response(),
- };
+ let git_repo = quire.repo(&repo_name)?;
let path_clone = path.clone();
let repo_d = repo_display.clone();
@@ -52,11 +55,14 @@ async fn tree_or_file_at_path(quire: Quire, repo: String, path: String, authed:
read_file_data(&reader, &path_clone).map(Err)
}
})
- .await
- .unwrap_or(None);
+ .await?;
+
+ let Some(result) = result else {
+ return Ok(StatusCode::NOT_FOUND.into_response());
+ };
- match result {
- Some(Ok((tree_data, recent_changes))) => {
+ Ok(match result {
+ Ok((tree_data, recent_changes)) => {
let crumbs = build_tree_crumbs(&repo_display, &path);
let tmpl = TreeTemplate {
sections: nav_sections(&repo_display, "tree", authed),
@@ -70,7 +76,7 @@ async fn tree_or_file_at_path(quire: Quire, repo: String, path: String, authed:
};
render(&tmpl)
}
- Some(Err(file_data)) => {
+ Err(file_data) => {
let crumbs = build_file_crumbs(&repo_display, &path);
let line_nums: Vec<usize> = (1..=file_data.line_count).collect();
let tmpl = FileViewTemplate {
@@ -99,8 +105,7 @@ async fn tree_or_file_at_path(quire: Quire, repo: String, path: String, authed:
};
render(&tmpl)
}
- None => StatusCode::NOT_FOUND.into_response(),
- }
+ })
}
// ── Tree (directory) view ──────────────────────────────────────
diff --git a/quire-server/src/quire/web/mod.rs b/quire-server/src/quire/web/mod.rs
index b9ded14..7bebfaa 100644
--- a/quire-server/src/quire/web/mod.rs
+++ b/quire-server/src/quire/web/mod.rs
@@ -9,6 +9,7 @@
pub mod api;
pub mod auth;
pub mod db;
+pub mod error;
pub mod format;
pub mod handlers;
pub mod templates;