Skip to content

Commit bbf2f87

Browse files
committed
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.
1 parent bfc44f9 commit bbf2f87

3 files changed

Lines changed: 341 additions & 7 deletions

File tree

‎crates/gitlawb-node/src/api/changelog.rs‎

Lines changed: 216 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,10 @@ pub async fn get_changelog(
4343
.await
4444
.map_err(|e| AppError::Git(e.to_string()))?;
4545
let head_ref = store::resolve_head(&disk_path, &record.default_branch);
46-
let commits = store::log(&disk_path, &head_ref, limit).unwrap_or_default();
46+
// A read failure is not an empty history: returning a bare 200 with no
47+
// events makes a degraded repo look identical to a brand-new one (#400).
48+
let commits =
49+
store::log(&disk_path, &head_ref, limit).map_err(|e| AppError::Git(e.to_string()))?;
4750

4851
let mut events: Vec<serde_json::Value> = commits
4952
.into_iter()
@@ -60,7 +63,9 @@ pub async fn get_changelog(
6063
.collect();
6164

6265
// ── Merged PRs ───────────────────────────────────────────────────────
63-
let prs = state.db.list_prs(&record.id).await.unwrap_or_default();
66+
// Same for the DB half: an outage must surface as an error (503 when the
67+
// pool is unreachable), not an empty timeline (#400).
68+
let prs = state.db.list_prs(&record.id).await?;
6469
for pr in prs.iter().filter(|p| p.status == "merged") {
6570
events.push(serde_json::json!({
6671
"type": "pr_merged",
@@ -88,3 +93,212 @@ pub async fn get_changelog(
8893
"count": events.len(),
8994
})))
9095
}
96+
97+
/// #400: endpoint-level proof that a degraded store or DB reaches the caller
98+
/// as an error, not a 200 with an empty timeline.
99+
#[cfg(test)]
100+
mod tests {
101+
use super::*;
102+
use axum::http::Request;
103+
use axum::http::StatusCode;
104+
use axum::Router;
105+
use sqlx::PgPool;
106+
use tempfile::TempDir;
107+
use tower::ServiceExt;
108+
109+
fn seed_repo(owner_did: &str, name: &str) -> crate::db::RepoRecord {
110+
let now = chrono::Utc::now();
111+
crate::db::RepoRecord {
112+
id: uuid::Uuid::new_v4().to_string(),
113+
name: name.to_string(),
114+
owner_did: owner_did.to_string(),
115+
description: None,
116+
is_public: true,
117+
default_branch: "main".to_string(),
118+
created_at: now,
119+
updated_at: now,
120+
disk_path: format!("/tmp/{name}"),
121+
forked_from: None,
122+
machine_id: None,
123+
}
124+
}
125+
126+
/// A state whose repo store roots in `repos_dir` so the test controls the
127+
/// on-disk repo, with the repo record already inserted.
128+
async fn seeded_state(
129+
pool: &PgPool,
130+
repos_dir: &std::path::Path,
131+
owner: &str,
132+
name: &str,
133+
) -> AppState {
134+
let mut state = crate::test_support::test_state(pool.clone()).await;
135+
state.repo_store =
136+
crate::git::repo_store::RepoStore::for_testing(repos_dir.to_path_buf(), pool.clone());
137+
state
138+
.db
139+
.create_repo(&seed_repo(owner, name))
140+
.await
141+
.expect("seed repo");
142+
state
143+
}
144+
145+
fn repo_disk_path(repos_dir: &std::path::Path, owner: &str, name: &str) -> std::path::PathBuf {
146+
repos_dir
147+
.join(owner.replace([':', '/'], "_"))
148+
.join(format!("{name}.git"))
149+
}
150+
151+
fn init_bare(path: &std::path::Path) {
152+
std::fs::create_dir_all(path).unwrap();
153+
let out = std::process::Command::new("git")
154+
.args(["init", "--bare"])
155+
.arg(path)
156+
.output()
157+
.unwrap();
158+
assert!(out.status.success());
159+
}
160+
161+
/// A bare repo with one real commit on HEAD.
162+
fn bare_repo_with_commit(
163+
repos_dir: &std::path::Path,
164+
owner: &str,
165+
name: &str,
166+
) -> std::path::PathBuf {
167+
let scratch = repos_dir.join(format!("scratch-{name}"));
168+
let out = std::process::Command::new("git")
169+
.args(["init"])
170+
.arg(&scratch)
171+
.output()
172+
.unwrap();
173+
assert!(out.status.success());
174+
let out = std::process::Command::new("git")
175+
.args([
176+
"-c",
177+
"user.email=t@t",
178+
"-c",
179+
"user.name=t",
180+
"commit",
181+
"--allow-empty",
182+
"-m",
183+
"initial",
184+
])
185+
.current_dir(&scratch)
186+
.output()
187+
.unwrap();
188+
assert!(out.status.success());
189+
let repo_path = repo_disk_path(repos_dir, owner, name);
190+
std::fs::create_dir_all(repo_path.parent().unwrap()).unwrap();
191+
let out = std::process::Command::new("git")
192+
.args(["clone", "--bare"])
193+
.arg(&scratch)
194+
.arg(&repo_path)
195+
.output()
196+
.unwrap();
197+
assert!(out.status.success());
198+
repo_path
199+
}
200+
201+
/// Delete the object behind HEAD and leave garbage: `git log` fails while
202+
/// `rev-parse` still resolves the ref.
203+
fn corrupt_head_object(repo_path: &std::path::Path) {
204+
let out = std::process::Command::new("git")
205+
.args(["rev-parse", "HEAD"])
206+
.current_dir(repo_path)
207+
.output()
208+
.unwrap();
209+
let oid = String::from_utf8(out.stdout).unwrap().trim().to_string();
210+
let obj = repo_path.join("objects").join(&oid[..2]).join(&oid[2..]);
211+
std::fs::remove_file(&obj).unwrap();
212+
std::fs::write(&obj, b"garbage").unwrap();
213+
}
214+
215+
async fn oneshot_changelog(
216+
state: AppState,
217+
owner: &str,
218+
name: &str,
219+
) -> axum::response::Response {
220+
Router::new()
221+
.route(
222+
"/api/v1/repos/{owner}/{repo}/changelog",
223+
axum::routing::get(get_changelog),
224+
)
225+
.with_state(state)
226+
.oneshot(
227+
Request::builder()
228+
.uri(format!("/api/v1/repos/{owner}/{name}/changelog"))
229+
.body(axum::body::Body::empty())
230+
.unwrap(),
231+
)
232+
.await
233+
.unwrap()
234+
}
235+
236+
/// The git half of the fold: a repo whose HEAD resolves but whose object
237+
/// store is corrupt must be a 500, not a 200 with zero events.
238+
#[sqlx::test]
239+
async fn changelog_on_corrupt_object_store_returns_500_not_empty_200(pool: PgPool) {
240+
let owner = "did:key:zCHANGELOGCORRUPTAAAAAAAAAAAAAAAAAAAA";
241+
let dir = TempDir::new().unwrap();
242+
let state = seeded_state(&pool, dir.path(), owner, "corrupt-log").await;
243+
let repo_path = bare_repo_with_commit(dir.path(), owner, "corrupt-log");
244+
corrupt_head_object(&repo_path);
245+
246+
let resp = oneshot_changelog(state, owner, "corrupt-log").await;
247+
assert_eq!(
248+
resp.status(),
249+
StatusCode::INTERNAL_SERVER_ERROR,
250+
"a resolving ref whose objects cannot be read is a git error"
251+
);
252+
}
253+
254+
/// The DB half of the fold: with the pull_requests table gone, list_prs
255+
/// fails and the endpoint must surface it (503) rather than answering a
256+
/// 200 with only the git-derived events.
257+
#[sqlx::test]
258+
async fn changelog_on_pr_table_failure_returns_error_not_empty_200(pool: PgPool) {
259+
let owner = "did:key:zCHANGELOGDBBBBBBBBBBBBBBBBBBBBBBBBBBB";
260+
let dir = TempDir::new().unwrap();
261+
let state = seeded_state(&pool, dir.path(), owner, "db-fail").await;
262+
init_bare(&repo_disk_path(dir.path(), owner, "db-fail"));
263+
sqlx::query("DROP TABLE pull_requests")
264+
.execute(&pool)
265+
.await
266+
.unwrap();
267+
268+
let resp = oneshot_changelog(state, owner, "db-fail").await;
269+
assert!(
270+
resp.status().is_server_error(),
271+
"a DB failure must not answer a 200 empty timeline; got {}",
272+
resp.status()
273+
);
274+
}
275+
276+
/// Must-not direction: a healthy repo with a commit still returns the
277+
/// event; an empty repo is still a valid empty timeline.
278+
#[sqlx::test]
279+
async fn changelog_still_serves_commit_and_empty_repo(pool: PgPool) {
280+
let owner = "did:key:zCHANGELOGOKCCCCCCCCCCCCCCCCCCCCCCCCCCC";
281+
let dir = TempDir::new().unwrap();
282+
let state = seeded_state(&pool, dir.path(), owner, "with-commit").await;
283+
bare_repo_with_commit(dir.path(), owner, "with-commit");
284+
285+
let resp = oneshot_changelog(state, owner, "with-commit").await;
286+
assert_eq!(resp.status(), StatusCode::OK);
287+
let bytes = axum::body::to_bytes(resp.into_body(), usize::MAX)
288+
.await
289+
.unwrap();
290+
let v: serde_json::Value = serde_json::from_slice(&bytes).unwrap();
291+
assert_eq!(v["count"], 1, "the seeded commit must appear");
292+
assert_eq!(v["events"][0]["type"], "commit");
293+
294+
let state = seeded_state(&pool, dir.path(), owner, "empty-repo").await;
295+
init_bare(&repo_disk_path(dir.path(), owner, "empty-repo"));
296+
let resp = oneshot_changelog(state, owner, "empty-repo").await;
297+
assert_eq!(resp.status(), StatusCode::OK);
298+
let bytes = axum::body::to_bytes(resp.into_body(), usize::MAX)
299+
.await
300+
.unwrap();
301+
let v: serde_json::Value = serde_json::from_slice(&bytes).unwrap();
302+
assert_eq!(v["count"], 0, "a genuinely empty repo is still 200/empty");
303+
}
304+
}

‎crates/gitlawb-node/src/git/store.rs‎

Lines changed: 60 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -141,7 +141,21 @@ pub fn log(repo_path: &Path, refname: &str, limit: usize) -> Result<Vec<CommitIn
141141
.context("failed to run git log")?;
142142

143143
if !output.status.success() {
144-
return Ok(vec![]); // empty repo
144+
// An unresolvable ref means a genuinely empty repo. A ref that
145+
// resolves but fails to log is a read failure (corrupt object store, a
146+
// gc mid-read): report it rather than passing an empty log off as no
147+
// history (#400). rev-parse resolves the name without reading objects.
148+
// allow-unbounded-git: failure-path existence recheck; module
149+
// convention, at most one extra spawn per failed log.
150+
let resolved = Command::new("git")
151+
.args(["rev-parse", "--verify", "--quiet", refname])
152+
.current_dir(repo_path)
153+
.output();
154+
if matches!(resolved, Ok(ref o) if o.status.success()) {
155+
let stderr = String::from_utf8_lossy(&output.stderr);
156+
anyhow::bail!("git log failed for {refname}: {}", stderr.trim());
157+
}
158+
return Ok(vec![]);
145159
}
146160

147161
let stdout = String::from_utf8_lossy(&output.stdout);
@@ -2025,4 +2039,49 @@ mod tests {
20252039
"a clean `missing` twice on a readable store is a genuine absence; got {res:?}"
20262040
);
20272041
}
2042+
2043+
// #400: `log` may fold a failed `git log` into Ok(vec![]) only when the ref
2044+
// genuinely does not resolve (an empty repo). A ref that resolves but
2045+
// fails to read is an error, not an empty history.
2046+
#[test]
2047+
fn log_empty_is_empty_and_resolved_ref_read_failure_errors() {
2048+
let td = tempfile::TempDir::new().unwrap();
2049+
let work: &Path = td.path();
2050+
let g = |args: &[&str]| {
2051+
assert!(Command::new("git")
2052+
.args(args)
2053+
.current_dir(work)
2054+
.status()
2055+
.unwrap()
2056+
.success());
2057+
};
2058+
g(&["init", "-q"]);
2059+
g(&["config", "user.email", "t@t"]);
2060+
g(&["config", "user.name", "t"]);
2061+
2062+
// No commits: nothing resolves, so a failed log is a real empty repo.
2063+
assert!(super::log(work, "HEAD", 10).unwrap().is_empty());
2064+
2065+
std::fs::write(work.join("f.txt"), b"x\n").unwrap();
2066+
g(&["add", "."]);
2067+
g(&["commit", "-qm", "c1"]);
2068+
assert_eq!(super::log(work, "HEAD", 10).unwrap().len(), 1);
2069+
2070+
// Corrupt the object the branch resolves to: `git log` fails but
2071+
// `rev-parse --verify` still resolves the name, so the failure must
2072+
// surface as an error rather than an empty history.
2073+
let oid = {
2074+
let o = Command::new("git")
2075+
.args(["rev-parse", "HEAD"])
2076+
.current_dir(work)
2077+
.output()
2078+
.unwrap();
2079+
String::from_utf8_lossy(&o.stdout).trim().to_string()
2080+
};
2081+
let obj = work.join(".git/objects").join(&oid[..2]).join(&oid[2..]);
2082+
std::fs::remove_file(&obj).unwrap();
2083+
std::fs::write(&obj, b"garbage").unwrap();
2084+
2085+
assert!(super::log(work, "HEAD", 10).is_err());
2086+
}
20282087
}

0 commit comments

Comments
 (0)