Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions crates/gitlawb-node/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ sqlx = { version = "0.8", features = ["postgres", "runtime-tokio-rustls", "chron
clap = { version = "4", features = ["derive", "env"] }
bytes = "1"
libc = "0.2"
async-trait = "0.1"
cid = { workspace = true }
hex = { workspace = true }
sha2 = { workspace = true }
Expand Down
26 changes: 19 additions & 7 deletions crates/gitlawb-node/src/api/issues.rs
Original file line number Diff line number Diff line change
Expand Up @@ -70,10 +70,17 @@ pub async fn create_issue(

let create_result = git_issues::create_issue(&disk_path, &issue_id, &json_str);

// Always release the advisory lock — even on error; upload to Tigris only on success.
guard.release(create_result.is_ok()).await;
// Always release the advisory lock — even on error; upload to storage only on success.
let release_result = guard.release(create_result.is_ok()).await;

create_result.map_err(|e| AppError::Git(e.to_string()))?;
// The write succeeded locally; surface a durable-upload failure rather than
// acking an issue that never reached storage.
release_result.map_err(|e| {
AppError::Git(format!(
"issue stored locally but durable upload failed: {e}"
))
})?;

// Bump trust score for the issue author — increment current score by 0.05
// (avoids the push_count=0 stuck-at-0.05 bug for agents who only file issues)
Expand Down Expand Up @@ -229,11 +236,11 @@ pub async fn close_issue(
.ok()
.and_then(|i| i.author),
Ok(None) => {
guard.release(false).await;
let _ = guard.release(false).await;
return Err(AppError::NotFound(format!("issue {issue_id} not found")));
}
Err(e) => {
guard.release(false).await;
let _ = guard.release(false).await;
return Err(AppError::Git(e.to_string()));
}
};
Expand All @@ -242,20 +249,25 @@ pub async fn close_issue(
.as_deref()
.is_some_and(|a| crate::api::did_matches(&auth.0, a));
if !is_owner && !is_author {
guard.release(false).await;
let _ = guard.release(false).await;
return Err(AppError::Forbidden(
"only the repo owner or the issue author can close this issue".into(),
));
}

let close_result = git_issues::close_issue(&disk_path, &issue_id);

// Always release the advisory lock — even on error; upload to Tigris only on success.
guard.release(close_result.is_ok()).await;
// Always release the advisory lock — even on error; upload to storage only on success.
let release_result = guard.release(close_result.is_ok()).await;

let updated = close_result
.map_err(|e| AppError::Git(e.to_string()))?
.ok_or_else(|| AppError::RepoNotFound(format!("issue {issue_id} not found")))?;
release_result.map_err(|e| {
AppError::Git(format!(
"issue stored locally but durable upload failed: {e}"
))
})?;

let issue: serde_json::Value = serde_json::from_str(&updated)
.map_err(|e| AppError::BadRequest(format!("invalid issue data: {e}")))?;
Expand Down
11 changes: 9 additions & 2 deletions crates/gitlawb-node/src/api/pulls.rs
Original file line number Diff line number Diff line change
Expand Up @@ -224,10 +224,17 @@ pub async fn merge_pr(
&pr.title,
);

// Always release the advisory lock — even on error; upload to Tigris only on success.
guard.release(merge_result.is_ok()).await;
// Always release the advisory lock — even on error; upload to storage only on success.
let release_result = guard.release(merge_result.is_ok()).await;

let merge_sha = merge_result.map_err(|e| AppError::Git(e.to_string()))?;
// Surface a durable-upload failure rather than acking a merge that never
// reached storage.
release_result.map_err(|e| {
AppError::Git(format!(
"merge applied locally but durable upload failed: {e}"
))
})?;

state.db.merge_pr(&pr.id, &merger_did).await?;
let _ = state.db.touch_repo(&record.id).await;
Expand Down
45 changes: 39 additions & 6 deletions crates/gitlawb-node/src/api/repos.rs
Original file line number Diff line number Diff line change
Expand Up @@ -376,7 +376,7 @@ pub async fn git_info_refs(
}
}

// For receive-pack (push), download the latest from Tigris so the client
// For receive-pack (push), download the latest from storage so the client
// sees the same refs that acquire_write() will operate on.
let disk_path = if service == "git-receive-pack" {
state
Expand Down Expand Up @@ -549,9 +549,36 @@ pub async fn git_receive_pack(
let receive_result = smart_http::receive_pack(&disk_path, body).await;

// Always release the advisory lock — even on error — to prevent stale locks
// from blocking subsequent pushes. Only upload to Tigris when the push
// from blocking subsequent pushes. Only upload to storage when the push
// succeeded; uploading a half-applied repo would propagate corruption.
guard.release(receive_result.is_ok()).await;
let push_ok = receive_result.is_ok();
if push_ok && state.config.async_upload {
// Write-back: ack the client now; the durable upload to object storage
// and the advisory-lock release run in the background. The lock is held
// until the upload finishes, so a concurrent writer on another machine
// can't observe a stale archive. Durability tradeoff: if the node stops
// after this ack but before the upload, the local copy survives but
// STORAGE stays stale until this repo's next successful push re-uploads
// — it is not automatically reconciled. Hence async_upload is opt-in.
let repo_label = name.to_string();
tokio::spawn(async move {
if let Err(e) = guard.release(true).await {
tracing::error!(repo = %repo_label, err = %e,
"write-back durable upload failed after push was acked");
}
});
} else {
// Strict path (or failed push): upload-before-ack. On a successful push
// whose durable upload then fails, surface an error so the client knows
// the push is not durably stored (a failed push returns Ok from release
// and the push error is reported below).
if let Err(e) = guard.release(push_ok).await {
tracing::error!(repo = %name, err = %e, "durable upload failed after push");
return Err(AppError::Git(format!(
"push applied locally but durable upload to storage failed: {e}"
)));
}
}

let result = receive_result.map_err(|e| {
tracing::error!(repo = %name, err = %e, "git receive-pack failed");
Expand Down Expand Up @@ -1130,7 +1157,7 @@ pub async fn fork_repo(
)));
}

// Ensure source repo is on local disk (downloads from Tigris on cache miss)
// Ensure source repo is on local disk (downloads from storage on cache miss)
let source_path = state
.repo_store
.acquire(&source.owner_did, &source.name)
Expand All @@ -1157,11 +1184,17 @@ pub async fn fork_repo(
)));
}

// Upload fork to Tigris
// Upload fork to storage — fail closed if the durable upload fails rather
// than reporting a fork that only exists on this node's local disk.
state
.repo_store
.release_after_write(&forker_did, &fork_name)
.await;
.await
.map_err(|e| {
AppError::Git(format!(
"fork created locally but durable upload failed: {e}"
))
})?;
Comment on lines +1187 to +1197

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Clean up the local fork directory on durable-upload failure.

If release_after_write(...) fails, this returns an error but leaves the cloned disk_path behind. That creates orphan data and can make retries fail due to an existing destination path.

♻️ Proposed fix
-    state
-        .repo_store
-        .release_after_write(&forker_did, &fork_name)
-        .await
-        .map_err(|e| {
-            AppError::Git(format!(
-                "fork created locally but durable upload failed: {e}"
-            ))
-        })?;
+    if let Err(e) = state
+        .repo_store
+        .release_after_write(&forker_did, &fork_name)
+        .await
+    {
+        if let Err(cleanup_err) = tokio::fs::remove_dir_all(&disk_path).await {
+            tracing::warn!(
+                path = %disk_path.display(),
+                err = %cleanup_err,
+                "failed to clean up local fork after durable upload failure"
+            );
+        }
+        return Err(AppError::Git(format!(
+            "fork created locally but durable upload failed: {e}"
+        )));
+    }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/gitlawb-node/src/api/repos.rs` around lines 1187 - 1197, In the error
handler for the release_after_write call (within the map_err block), add cleanup
logic to remove the local fork directory stored in disk_path before returning
the AppError. This prevents orphan data from accumulating when the durable
upload fails, which can cause issues on retry attempts due to the destination
path already existing. The cleanup should happen in the error path before
constructing and returning the AppError with the failure message.


let now = Utc::now();
let record = crate::db::RepoRecord {
Expand Down
37 changes: 36 additions & 1 deletion crates/gitlawb-node/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -121,11 +121,46 @@ pub struct Config {
#[arg(long, env = "GITLAWB_HEARTBEAT_INTERVAL_HOURS", default_value_t = 20)]
pub heartbeat_interval_hours: u64,

/// Tigris (S3-compatible) bucket for repo storage.
/// Tigris (S3-compatible) bucket for repo storage. Legacy alias for
/// `s3_bucket` — still honoured so existing deployments keep working.
/// Leave empty to disable Tigris and use local-only storage.
#[arg(long, env = "GITLAWB_TIGRIS_BUCKET", default_value = "")]
pub tigris_bucket: String,

/// Object-storage backend: `s3` (any S3-compatible service), `fs` (local
/// directory), or `ipfs` (Kubo MFS). Empty = auto-detect: `s3` when a bucket
/// is set, else `fs` when a storage dir is set, else `ipfs` when an IPFS API
/// is set, else local-only.
#[arg(long, env = "GITLAWB_STORAGE_BACKEND", default_value = "")]
pub storage_backend: String,

/// Bucket for the `s3` backend (Tigris, R2, AWS S3, MinIO, B2). Falls back to
/// `tigris_bucket` when empty.
#[arg(long, env = "GITLAWB_S3_BUCKET", default_value = "")]
pub s3_bucket: String,

/// Endpoint URL override for the `s3` backend (e.g. R2/MinIO). On Tigris/Fly
/// the endpoint is auto-provided via `AWS_ENDPOINT_URL_S3`, so leave empty.
#[arg(long, env = "GITLAWB_S3_ENDPOINT", default_value = "")]
pub s3_endpoint: String,

/// Force path-style S3 addressing (required by MinIO and some S3-compatibles).
#[arg(long, env = "GITLAWB_S3_FORCE_PATH_STYLE", default_value_t = false)]
pub s3_force_path_style: bool,

/// Directory for the `fs` (local filesystem) storage backend.
#[arg(long, env = "GITLAWB_STORAGE_FS_DIR", default_value = "")]
pub storage_fs_dir: String,

/// Acknowledge a push to the client before the durable upload to object
/// storage finishes (write-back). Lowers push latency, but opens a
/// durability window: if the node stops between the ack and the upload, on
/// restart a stale remote archive can overwrite the newer local copy. Off
/// by default (strict upload-before-ack); opt in only where the latency win
/// is worth that risk.
#[arg(long, env = "GITLAWB_ASYNC_UPLOAD", default_value_t = false)]
pub async_upload: bool,
Comment thread
coderabbitai[bot] marked this conversation as resolved.

/// Maximum pack body size for git-receive-pack and git-upload-pack, in bytes.
/// Applies only to git smart-HTTP routes — all other API routes keep the 2 MB default.
/// Default: 2 GB. Set lower on resource-constrained nodes.
Expand Down
5 changes: 0 additions & 5 deletions crates/gitlawb-node/src/db/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -246,11 +246,6 @@ pub struct Db {
}

impl Db {
/// Access the underlying Postgres connection pool.
pub fn pool(&self) -> &PgPool {
&self.pool
}

#[cfg(test)]
pub fn for_testing(pool: PgPool) -> Self {
Self { pool }
Expand Down
1 change: 0 additions & 1 deletion crates/gitlawb-node/src/git/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,5 +2,4 @@ pub mod issues;
pub mod repo_store;
pub mod smart_http;
pub mod store;
pub mod tigris;
pub mod visibility_pack;
Loading
Loading