diff --git a/crates/gitlawb-node/src/api/issues.rs b/crates/gitlawb-node/src/api/issues.rs index 9aeba20a..63134e7a 100644 --- a/crates/gitlawb-node/src/api/issues.rs +++ b/crates/gitlawb-node/src/api/issues.rs @@ -70,8 +70,8 @@ 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 - guard.release().await; + // Always release the advisory lock — even on error; upload to Tigris only on success. + guard.release(create_result.is_ok()).await; create_result.map_err(|e| AppError::Git(e.to_string()))?; @@ -225,8 +225,8 @@ pub async fn close_issue( let close_result = git_issues::close_issue(&disk_path, &issue_id); - // Always release the advisory lock — even on error - guard.release().await; + // Always release the advisory lock — even on error; upload to Tigris only on success. + guard.release(close_result.is_ok()).await; let updated = close_result .map_err(|e| AppError::Git(e.to_string()))? diff --git a/crates/gitlawb-node/src/api/pulls.rs b/crates/gitlawb-node/src/api/pulls.rs index 865f53ea..419a932f 100644 --- a/crates/gitlawb-node/src/api/pulls.rs +++ b/crates/gitlawb-node/src/api/pulls.rs @@ -203,8 +203,8 @@ pub async fn merge_pr( &pr.title, ); - // Always release the advisory lock — even on error - guard.release().await; + // Always release the advisory lock — even on error; upload to Tigris only on success. + guard.release(merge_result.is_ok()).await; let merge_sha = merge_result.map_err(|e| AppError::Git(e.to_string()))?; diff --git a/crates/gitlawb-node/src/api/repos.rs b/crates/gitlawb-node/src/api/repos.rs index 0993d4bc..ef8f5982 100644 --- a/crates/gitlawb-node/src/api/repos.rs +++ b/crates/gitlawb-node/src/api/repos.rs @@ -484,8 +484,9 @@ 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 to the same repo. - guard.release().await; + // from blocking subsequent pushes. Only upload to Tigris when the push + // succeeded; uploading a half-applied repo would propagate corruption. + guard.release(receive_result.is_ok()).await; let result = receive_result.map_err(|e| { tracing::error!(repo = %name, err = %e, "git receive-pack failed"); diff --git a/crates/gitlawb-node/src/git/repo_store.rs b/crates/gitlawb-node/src/git/repo_store.rs index deab29ce..a5c367e9 100644 --- a/crates/gitlawb-node/src/git/repo_store.rs +++ b/crates/gitlawb-node/src/git/repo_store.rs @@ -130,10 +130,19 @@ impl RepoStore { if let Some(ref tigris) = self.tigris { if tigris.exists(&owner_slug, repo_name).await.unwrap_or(false) { debug!(repo = %repo_name, "acquire_fresh: downloading latest from tigris"); - tigris - .download(&owner_slug, repo_name, &local_path) - .await - .context("downloading repo from tigris (fresh)")?; + if let Err(e) = tigris.download(&owner_slug, repo_name, &local_path).await { + // The Tigris archive is present (HEAD ok) but unreadable — a + // corrupt/partial upload, or a transient GET failure. If we have a + // valid local copy, proceed with it rather than blocking the write; + // the post-write upload re-syncs (self-heals) Tigris. Only hard-fail + // when there is no local copy to fall back to. + if local_path.exists() { + warn!(repo = %repo_name, err = %e, + "acquire_fresh: tigris download failed — falling back to local copy"); + return Ok(local_path); + } + return Err(e).context("downloading repo from tigris (fresh)"); + } return Ok(local_path); } } @@ -174,10 +183,17 @@ impl RepoStore { if let Some(ref tigris) = self.tigris { if tigris.exists(&owner_slug, repo_name).await.unwrap_or(false) { debug!(repo = %repo_name, "write acquire: downloading latest from tigris"); - tigris - .download(&owner_slug, repo_name, &local_path) - .await - .context("downloading repo from tigris for write")?; + if let Err(e) = tigris.download(&owner_slug, repo_name, &local_path).await { + // Same self-healing fallback as acquire_fresh: a corrupt/unreadable + // Tigris archive must not block a write when a valid local copy + // exists — release(success) will re-upload a good archive. + if local_path.exists() { + warn!(repo = %repo_name, err = %e, + "write acquire: tigris download failed — falling back to local copy"); + } else { + return Err(e).context("downloading repo from tigris for write"); + } + } } } @@ -348,16 +364,24 @@ impl RepoWriteGuard { &self.local_path } - /// Upload to Tigris and release the advisory lock. Call this when the write is done. - pub async fn release(self) { - // Upload to Tigris - if let Some(ref tigris) = self.tigris { - if let Err(e) = tigris - .upload(&self.owner_slug, &self.repo_name, &self.local_path) - .await - { - warn!(repo = %self.repo_name, err = %e, "failed to upload repo to tigris after write"); + /// Upload to Tigris (only when the write succeeded) and release the advisory + /// lock. Pass `success = false` when the write operation failed — uploading a + /// half-applied or otherwise inconsistent repo would propagate corruption to + /// Tigris (and to every node that later downloads it). The lock is always + /// released regardless, to avoid stale locks blocking future writes. + pub async fn release(self, success: bool) { + // Upload to Tigris only on success. + if success { + if let Some(ref tigris) = self.tigris { + if let Err(e) = tigris + .upload(&self.owner_slug, &self.repo_name, &self.local_path) + .await + { + warn!(repo = %self.repo_name, err = %e, "failed to upload repo to tigris after write"); + } } + } else { + warn!(repo = %self.repo_name, "write failed — skipping tigris upload to avoid propagating an inconsistent repo"); } // Release advisory lock diff --git a/crates/gitlawb-node/src/git/tigris.rs b/crates/gitlawb-node/src/git/tigris.rs index 966c5ce8..60f7da38 100644 --- a/crates/gitlawb-node/src/git/tigris.rs +++ b/crates/gitlawb-node/src/git/tigris.rs @@ -157,16 +157,46 @@ fn compress_repo(repo_path: &Path) -> Result> { } /// Decompress a tar.zst byte vector into a local directory. +/// +/// Extraction is atomic with respect to `local_path`: the archive is unpacked +/// into a sibling temp directory first, and only swapped into place once it +/// fully succeeds. A corrupt or truncated archive therefore can never clobber a +/// good existing copy at `local_path` — on failure we discard the temp dir and +/// leave `local_path` exactly as it was. fn decompress_repo(data: &[u8], local_path: &Path) -> Result<()> { - // Ensure parent directory exists - if let Some(parent) = local_path.parent() { - std::fs::create_dir_all(parent).context("creating parent dir")?; + let parent = local_path.parent().context("repo path has no parent")?; + std::fs::create_dir_all(parent).context("creating parent dir")?; + + let file_name = local_path + .file_name() + .context("repo path has no file name")? + .to_string_lossy(); + let tmp_dir = parent.join(format!(".{file_name}.tmp-extract")); + + // Clear any leftover temp dir from a previously-interrupted extraction. + let _ = std::fs::remove_dir_all(&tmp_dir); + std::fs::create_dir_all(&tmp_dir).context("creating temp extract dir")?; + + // Unpack into the temp dir; on any failure, clean up and bail without + // touching local_path. + let unpack = (|| -> Result<()> { + let decoder = zstd::stream::Decoder::new(data)?; + let mut archive = tar::Archive::new(decoder); + archive.unpack(&tmp_dir).context("unpacking tar.zst")?; + Ok(()) + })(); + if let Err(e) = unpack { + let _ = std::fs::remove_dir_all(&tmp_dir); + return Err(e); } - std::fs::create_dir_all(local_path).context("creating repo dir")?; - let decoder = zstd::stream::Decoder::new(data)?; - let mut archive = tar::Archive::new(decoder); - archive.unpack(local_path).context("unpacking tar.zst")?; + // Swap the freshly-extracted repo into place. rename within the same parent + // is effectively atomic, but most platforms refuse to rename onto a + // non-empty dir, so remove the old copy first. + if local_path.exists() { + std::fs::remove_dir_all(local_path).context("removing stale repo dir")?; + } + std::fs::rename(&tmp_dir, local_path).context("swapping extracted repo into place")?; Ok(()) }