From bbf2f876c87e15651eb3205d7110a9da241ed33c Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Thu, 10 Sep 2026 19:17:16 -0500 Subject: [PATCH 1/2] fix: warn or error instead of returning silent empty results Three sites folded failures into empty results indistinguishable from genuine emptiness: - clone.rs encrypted-blobs list fetch: a non-2xx or transport error silently returned no recovered paths; now warns via emit_warning, matching the per-blob stage in the same function. - clone.rs arweave fallback: unwrap_or_default() became unwrap_or_else that warns, matching the node-recovery arm directly above it. - changelog handler: store::log failures now return a git error and list_prs failures propagate (503 when the pool is unreachable) instead of answering 200 with an empty timeline. store::log itself only returns empty when the ref genuinely does not resolve: a ref that resolves but fails to log is a read failure, not an empty repo. Closes #400. --- crates/gitlawb-node/src/api/changelog.rs | 218 ++++++++++++++++++++++- crates/gitlawb-node/src/git/store.rs | 61 ++++++- crates/gl/src/clone.rs | 69 ++++++- 3 files changed, 341 insertions(+), 7 deletions(-) diff --git a/crates/gitlawb-node/src/api/changelog.rs b/crates/gitlawb-node/src/api/changelog.rs index 40e52cb4..b6851b7b 100644 --- a/crates/gitlawb-node/src/api/changelog.rs +++ b/crates/gitlawb-node/src/api/changelog.rs @@ -43,7 +43,10 @@ pub async fn get_changelog( .await .map_err(|e| AppError::Git(e.to_string()))?; let head_ref = store::resolve_head(&disk_path, &record.default_branch); - let commits = store::log(&disk_path, &head_ref, limit).unwrap_or_default(); + // A read failure is not an empty history: returning a bare 200 with no + // events makes a degraded repo look identical to a brand-new one (#400). + let commits = + store::log(&disk_path, &head_ref, limit).map_err(|e| AppError::Git(e.to_string()))?; let mut events: Vec = commits .into_iter() @@ -60,7 +63,9 @@ pub async fn get_changelog( .collect(); // ── Merged PRs ─────────────────────────────────────────────────────── - let prs = state.db.list_prs(&record.id).await.unwrap_or_default(); + // Same for the DB half: an outage must surface as an error (503 when the + // pool is unreachable), not an empty timeline (#400). + let prs = state.db.list_prs(&record.id).await?; for pr in prs.iter().filter(|p| p.status == "merged") { events.push(serde_json::json!({ "type": "pr_merged", @@ -88,3 +93,212 @@ pub async fn get_changelog( "count": events.len(), }))) } + +/// #400: endpoint-level proof that a degraded store or DB reaches the caller +/// as an error, not a 200 with an empty timeline. +#[cfg(test)] +mod tests { + use super::*; + use axum::http::Request; + use axum::http::StatusCode; + use axum::Router; + use sqlx::PgPool; + use tempfile::TempDir; + use tower::ServiceExt; + + fn seed_repo(owner_did: &str, name: &str) -> crate::db::RepoRecord { + let now = chrono::Utc::now(); + crate::db::RepoRecord { + id: uuid::Uuid::new_v4().to_string(), + name: name.to_string(), + owner_did: owner_did.to_string(), + description: None, + is_public: true, + default_branch: "main".to_string(), + created_at: now, + updated_at: now, + disk_path: format!("/tmp/{name}"), + forked_from: None, + machine_id: None, + } + } + + /// A state whose repo store roots in `repos_dir` so the test controls the + /// on-disk repo, with the repo record already inserted. + async fn seeded_state( + pool: &PgPool, + repos_dir: &std::path::Path, + owner: &str, + name: &str, + ) -> AppState { + let mut state = crate::test_support::test_state(pool.clone()).await; + state.repo_store = + crate::git::repo_store::RepoStore::for_testing(repos_dir.to_path_buf(), pool.clone()); + state + .db + .create_repo(&seed_repo(owner, name)) + .await + .expect("seed repo"); + state + } + + fn repo_disk_path(repos_dir: &std::path::Path, owner: &str, name: &str) -> std::path::PathBuf { + repos_dir + .join(owner.replace([':', '/'], "_")) + .join(format!("{name}.git")) + } + + fn init_bare(path: &std::path::Path) { + std::fs::create_dir_all(path).unwrap(); + let out = std::process::Command::new("git") + .args(["init", "--bare"]) + .arg(path) + .output() + .unwrap(); + assert!(out.status.success()); + } + + /// A bare repo with one real commit on HEAD. + fn bare_repo_with_commit( + repos_dir: &std::path::Path, + owner: &str, + name: &str, + ) -> std::path::PathBuf { + let scratch = repos_dir.join(format!("scratch-{name}")); + let out = std::process::Command::new("git") + .args(["init"]) + .arg(&scratch) + .output() + .unwrap(); + assert!(out.status.success()); + let out = std::process::Command::new("git") + .args([ + "-c", + "user.email=t@t", + "-c", + "user.name=t", + "commit", + "--allow-empty", + "-m", + "initial", + ]) + .current_dir(&scratch) + .output() + .unwrap(); + assert!(out.status.success()); + let repo_path = repo_disk_path(repos_dir, owner, name); + std::fs::create_dir_all(repo_path.parent().unwrap()).unwrap(); + let out = std::process::Command::new("git") + .args(["clone", "--bare"]) + .arg(&scratch) + .arg(&repo_path) + .output() + .unwrap(); + assert!(out.status.success()); + repo_path + } + + /// Delete the object behind HEAD and leave garbage: `git log` fails while + /// `rev-parse` still resolves the ref. + fn corrupt_head_object(repo_path: &std::path::Path) { + let out = std::process::Command::new("git") + .args(["rev-parse", "HEAD"]) + .current_dir(repo_path) + .output() + .unwrap(); + let oid = String::from_utf8(out.stdout).unwrap().trim().to_string(); + let obj = repo_path.join("objects").join(&oid[..2]).join(&oid[2..]); + std::fs::remove_file(&obj).unwrap(); + std::fs::write(&obj, b"garbage").unwrap(); + } + + async fn oneshot_changelog( + state: AppState, + owner: &str, + name: &str, + ) -> axum::response::Response { + Router::new() + .route( + "/api/v1/repos/{owner}/{repo}/changelog", + axum::routing::get(get_changelog), + ) + .with_state(state) + .oneshot( + Request::builder() + .uri(format!("/api/v1/repos/{owner}/{name}/changelog")) + .body(axum::body::Body::empty()) + .unwrap(), + ) + .await + .unwrap() + } + + /// The git half of the fold: a repo whose HEAD resolves but whose object + /// store is corrupt must be a 500, not a 200 with zero events. + #[sqlx::test] + async fn changelog_on_corrupt_object_store_returns_500_not_empty_200(pool: PgPool) { + let owner = "did:key:zCHANGELOGCORRUPTAAAAAAAAAAAAAAAAAAAA"; + let dir = TempDir::new().unwrap(); + let state = seeded_state(&pool, dir.path(), owner, "corrupt-log").await; + let repo_path = bare_repo_with_commit(dir.path(), owner, "corrupt-log"); + corrupt_head_object(&repo_path); + + let resp = oneshot_changelog(state, owner, "corrupt-log").await; + assert_eq!( + resp.status(), + StatusCode::INTERNAL_SERVER_ERROR, + "a resolving ref whose objects cannot be read is a git error" + ); + } + + /// The DB half of the fold: with the pull_requests table gone, list_prs + /// fails and the endpoint must surface it (503) rather than answering a + /// 200 with only the git-derived events. + #[sqlx::test] + async fn changelog_on_pr_table_failure_returns_error_not_empty_200(pool: PgPool) { + let owner = "did:key:zCHANGELOGDBBBBBBBBBBBBBBBBBBBBBBBBBBB"; + let dir = TempDir::new().unwrap(); + let state = seeded_state(&pool, dir.path(), owner, "db-fail").await; + init_bare(&repo_disk_path(dir.path(), owner, "db-fail")); + sqlx::query("DROP TABLE pull_requests") + .execute(&pool) + .await + .unwrap(); + + let resp = oneshot_changelog(state, owner, "db-fail").await; + assert!( + resp.status().is_server_error(), + "a DB failure must not answer a 200 empty timeline; got {}", + resp.status() + ); + } + + /// Must-not direction: a healthy repo with a commit still returns the + /// event; an empty repo is still a valid empty timeline. + #[sqlx::test] + async fn changelog_still_serves_commit_and_empty_repo(pool: PgPool) { + let owner = "did:key:zCHANGELOGOKCCCCCCCCCCCCCCCCCCCCCCCCCCC"; + let dir = TempDir::new().unwrap(); + let state = seeded_state(&pool, dir.path(), owner, "with-commit").await; + bare_repo_with_commit(dir.path(), owner, "with-commit"); + + let resp = oneshot_changelog(state, owner, "with-commit").await; + assert_eq!(resp.status(), StatusCode::OK); + let bytes = axum::body::to_bytes(resp.into_body(), usize::MAX) + .await + .unwrap(); + let v: serde_json::Value = serde_json::from_slice(&bytes).unwrap(); + assert_eq!(v["count"], 1, "the seeded commit must appear"); + assert_eq!(v["events"][0]["type"], "commit"); + + let state = seeded_state(&pool, dir.path(), owner, "empty-repo").await; + init_bare(&repo_disk_path(dir.path(), owner, "empty-repo")); + let resp = oneshot_changelog(state, owner, "empty-repo").await; + assert_eq!(resp.status(), StatusCode::OK); + let bytes = axum::body::to_bytes(resp.into_body(), usize::MAX) + .await + .unwrap(); + let v: serde_json::Value = serde_json::from_slice(&bytes).unwrap(); + assert_eq!(v["count"], 0, "a genuinely empty repo is still 200/empty"); + } +} diff --git a/crates/gitlawb-node/src/git/store.rs b/crates/gitlawb-node/src/git/store.rs index 5617b419..0a640ab8 100644 --- a/crates/gitlawb-node/src/git/store.rs +++ b/crates/gitlawb-node/src/git/store.rs @@ -141,7 +141,21 @@ pub fn log(repo_path: &Path, refname: &str, limit: usize) -> Result Vec { + emit_warning(&format!("warning: {what} failed: {e}")); + Vec::new() +} + #[cfg(not(test))] fn warn_sink() -> impl std::io::Write { std::io::stderr() @@ -322,7 +329,19 @@ async fn recover_encrypted_blobs( .await { Ok(r) if r.status().is_success() => r, - _ => return Ok(vec![]), + Ok(r) => { + emit_warning(&format!( + "warning: could not list encrypted blobs for {owner}/{name}: node returned {}; skipping recovery", + r.status() + )); + return Ok(vec![]); + } + Err(e) => { + emit_warning(&format!( + "warning: could not list encrypted blobs for {owner}/{name}: {e}; skipping recovery" + )); + return Ok(vec![]); + } }; let body: EncryptedBlobsResponse = resp.json().await.context("parsing encrypted-blobs")?; if body.blobs.is_empty() { @@ -836,8 +855,7 @@ pub async fn run(args: CloneArgs) -> Result<()> { // fallback still runs), but the strict /encrypted-blobs parse now // fails closed on schema drift, so surface it rather than letting // `.unwrap_or_default()` silently swallow it into "no paths". - eprintln!("warning: encrypted-blobs recovery failed: {e}"); - Vec::new() + warn_recovery_failed("encrypted-blobs recovery", &e) }); let from_arweave = recover_from_arweave( &args.arweave_gateway, @@ -848,7 +866,10 @@ pub async fn run(args: CloneArgs) -> Result<()> { &keypair, ) .await - .unwrap_or_default(); + .unwrap_or_else(|e| { + // Same rule as the node-recovery arm above: warn, never abort. + warn_recovery_failed("arweave/ipfs gateway recovery", &e) + }); paths.extend(from_arweave); if !paths.is_empty() { @@ -929,6 +950,46 @@ mod tests { WARNINGS.with(|w| w.borrow().clone()) } + // #400: the recovery-arm degrade must warn, not silently become "no + // paths". `warn_recovery_failed` is the write both arms go through. + #[test] + fn warn_recovery_failed_warns_and_returns_empty() { + reset_warnings(); + let out = + super::warn_recovery_failed("arweave/ipfs gateway recovery", &anyhow::anyhow!("boom")); + assert!(out.is_empty()); + assert!( + warnings().contains("arweave/ipfs gateway recovery failed: boom"), + "a recovery failure must warn, got: {:?}", + warnings() + ); + } + + // #400: the encrypted-blobs LIST fetch must warn on failure instead of + // silently returning an empty recovery set. The per-blob stage already + // warns; the list fetch was the outlier. + #[tokio::test] + async fn encrypted_blobs_list_failure_warns_and_returns_empty() { + let mut server = mockito::Server::new_async().await; + let _m = server + .mock("GET", "/api/v1/repos/o/r/encrypted-blobs") + .with_status(500) + .create_async() + .await; + let kp = gitlawb_core::identity::Keypair::generate(); + let dest = TempDir::new().unwrap(); + reset_warnings(); + let paths = recover_encrypted_blobs(&server.url(), "o", "r", dest.path(), &kp) + .await + .unwrap(); + assert!(paths.is_empty()); + assert!( + warnings().contains("could not list encrypted blobs"), + "a failed list fetch must warn, got: {:?}", + warnings() + ); + } + fn g(args: &[&str], dir: &Path) { assert!(Command::new("git") .args(args) From d7da6aa159c39f64982edde6834742799de9882f Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Thu, 10 Sep 2026 21:29:26 -0500 Subject: [PATCH 2/2] fix(node): distinguish missing ref from probe failure in store::log --- crates/gitlawb-node/src/git/store.rs | 34 ++++++++++++++++++++++++---- 1 file changed, 29 insertions(+), 5 deletions(-) diff --git a/crates/gitlawb-node/src/git/store.rs b/crates/gitlawb-node/src/git/store.rs index 0a640ab8..fda4b9dc 100644 --- a/crates/gitlawb-node/src/git/store.rs +++ b/crates/gitlawb-node/src/git/store.rs @@ -150,12 +150,20 @@ pub fn log(repo_path: &Path, refname: &str, limit: usize) -> Result return Ok(vec![]), + Some(0) => {} + _ => { + let stderr = String::from_utf8_lossy(&resolved.stderr); + anyhow::bail!("git rev-parse failed for {refname}: {}", stderr.trim()); + } } - return Ok(vec![]); + let stderr = String::from_utf8_lossy(&output.stderr); + anyhow::bail!("git log failed for {refname}: {}", stderr.trim()); } let stdout = String::from_utf8_lossy(&output.stdout); @@ -2084,4 +2092,20 @@ mod tests { assert!(super::log(work, "HEAD", 10).is_err()); } + + /// The rev-parse recheck inside `log` must distinguish "ref missing" + /// (exit 1 -> empty history) from a real probe failure (exit 128 on a + /// non-repo -> error), not fold both into `Ok(vec![])`. + #[test] + fn log_probe_failure_on_a_non_repo_errors_instead_of_empty() { + let td = tempfile::TempDir::new().unwrap(); + // A directory that is not a git repo at all: `git log` fails and the + // `rev-parse` recheck fails with 128, which is not "missing ref". + let not_a_repo = td.path().join("not-a-repo"); + std::fs::create_dir_all(¬_a_repo).unwrap(); + assert!( + super::log(¬_a_repo, "HEAD", 10).is_err(), + "a rev-parse probe failure must not read as an empty repo" + ); + } }