Skip to content

Commit 97d37fd

Browse files
kevincodex1claude
andauthored
Perf/alloc and copy cleanups (#80)
* perf: trim allocations and redundant copies on hot paths - Cargo.toml: enable thin LTO + symbol stripping for release builds - git-remote-gitlawb: attach pack body after signing to move (not clone) potentially large pack bytes, halving peak push memory - gitlawb-attest: deserialize payload by reference instead of cloning it - core/http_sig: compare components by &str to avoid per-item String alloc - node/tigris: drop redundant to_vec() copy of the response body - node/rate_limit: only allocate the key String on first insert, not per request - node/sync: resolve the peer table once per batch instead of per item Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * perf(sync): skip peer lookup on empty sync batch Return early from process_batch when dequeue_pending_syncs yields nothing — the common case on the 30s idle poll — so an empty batch no longer fires an unnecessary list_peers() DB query (CodeRabbit, PR #80). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(sync): cover per-batch peer origin-URL resolution Extract the peer -> origin-URL lookup from process_batch into a pure resolve_origin_url() helper and unit-test it (match + trailing-slash trim, unknown DID, empty peer list). This is the per-batch resolution PR #80 introduced; pulling it out of the DB-bound process_batch lets it be tested without Postgres. No behavior change. Addresses the needs-tests label. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(tigris): use a unique temp dir per archive extraction The fixed `.{file_name}.tmp-extract` path let two concurrent extractions of the same repo share one temp dir, so one's remove_dir_all/unpack could clobber the other's in-progress work. Suffix the temp dir with a fresh UUID so each extraction is isolated; this also removes the need to pre-clear a leftover dir (a unique name can't collide with an interrupted run). The on-failure cleanup still removes our own dir, and success consumes it via rename (CodeRabbit). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(tigris): serialize repo publish per path to close the swap race Unique temp dirs let extractions unpack in parallel, but the final remove_dir_all + rename swap could still interleave for the same local_path, causing a nondeterministic overwrite or a rename-onto-non-empty failure. Guard just the swap with a process-wide mutex keyed by local_path so only one extraction publishes per repo at a time; the expensive unpack stays parallel (CodeRabbit). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(tigris): restore decompress_repo doc comment ordering The earlier publish_lock insertion landed between decompress_repo's doc comment and its fn, gluing the two doc blocks onto publish_lock and leaving decompress_repo undocumented. Reorder so each fn keeps its own doc. No behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(tigris): note publish_lock map is never evicted Document the known unbounded-growth limitation of the per-path LOCKS map: bounded by the repo count so negligible for normal use, but high-volume deployments may want LRU/weak-ref eviction. No behavior change (CodeRabbit). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent a8cc33a commit 97d37fd

7 files changed

Lines changed: 123 additions & 25 deletions

File tree

‎Cargo.toml‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,3 +48,7 @@ uuid = { version = "1", features = ["v4"] }
4848
reqwest = { version = "0.12", features = ["blocking", "json", "multipart", "rustls-tls"], default-features = false }
4949
# HMAC
5050
hmac = "0.12"
51+
52+
[profile.release]
53+
lto = "thin"
54+
strip = "symbols"

‎crates/git-remote-gitlawb/src/main.rs‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -272,8 +272,7 @@ fn handle_connect(
272272
let mut req = client
273273
.post(&post_url)
274274
.header("Content-Type", format!("application/x-{}-request", service))
275-
.header("User-Agent", "git/2.0 git-remote-gitlawb/0.1.0")
276-
.body(request_body.clone());
275+
.header("User-Agent", "git/2.0 git-remote-gitlawb/0.1.0");
277276

278277
// Add RFC 9421 HTTP Signature auth on push operations
279278
if service == "git-receive-pack" {
@@ -291,7 +290,12 @@ fn handle_connect(
291290
}
292291
}
293292

294-
let pack_resp = req.send().with_context(|| format!("POST {post_url}"))?;
293+
// Attach the body after signing so the pack bytes are moved, not cloned —
294+
// packs can be large and the clone doubled peak memory on push.
295+
let pack_resp = req
296+
.body(request_body)
297+
.send()
298+
.with_context(|| format!("POST {post_url}"))?;
295299

296300
if !pack_resp.status().is_success() {
297301
bail!("POST /{} returned {}", service, pack_resp.status());

‎crates/gitlawb-attest/src/attestation.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -127,7 +127,7 @@ impl Attestation {
127127
self.type_
128128
)));
129129
}
130-
Ok(serde_json::from_value(self.payload.clone())?)
130+
Ok(P::deserialize(&self.payload)?)
131131
}
132132
}
133133

‎crates/gitlawb-core/src/http_sig.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -129,7 +129,7 @@ impl HttpSignature {
129129
pub fn missing_components(&self) -> Vec<&str> {
130130
COVERED_COMPONENTS
131131
.iter()
132-
.filter(|c| !self.components.contains(&c.to_string()))
132+
.filter(|c| !self.components.iter().any(|s| s.as_str() == **c))
133133
.copied()
134134
.collect()
135135
}

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

Lines changed: 31 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,9 @@
33
//! Repos are stored as `repos/v1/{owner_slug}/{repo_name}.tar.zst` — a
44
//! zstd-compressed tar archive of the bare repo directory.
55
6-
use std::path::Path;
6+
use std::collections::HashMap;
7+
use std::path::{Path, PathBuf};
8+
use std::sync::{Arc, Mutex, OnceLock};
79

810
use anyhow::{Context, Result};
911
use aws_sdk_s3::Client as S3Client;
@@ -110,8 +112,7 @@ impl TigrisClient {
110112
.collect()
111113
.await
112114
.context("reading tigris response body")?
113-
.into_bytes()
114-
.to_vec();
115+
.into_bytes();
115116

116117
// Extract tar.zst to local path
117118
tokio::task::spawn_blocking({
@@ -156,6 +157,23 @@ fn compress_repo(repo_path: &Path) -> Result<Vec<u8>> {
156157
Ok(compressed)
157158
}
158159

160+
/// Per-repo-path lock serializing the publish (swap-into-place) step of
161+
/// `decompress_repo`. Concurrent extractions unpack into isolated temp dirs in
162+
/// parallel, but the final `remove_dir_all` + `rename` must not interleave for
163+
/// the same `local_path`, or they race to a nondeterministic overwrite/failure.
164+
fn publish_lock(local_path: &Path) -> Arc<Mutex<()>> {
165+
// KNOWN LIMITATION: this map is never evicted — one (PathBuf, Arc<Mutex>)
166+
// entry accrues per distinct repo path for the process lifetime. Bounded by
167+
// the number of repos a node hosts, so it's negligible for normal use, but
168+
// high-volume/churning deployments may want LRU or weak-ref eviction here.
169+
static LOCKS: OnceLock<Mutex<HashMap<PathBuf, Arc<Mutex<()>>>>> = OnceLock::new();
170+
let locks = LOCKS.get_or_init(|| Mutex::new(HashMap::new()));
171+
let mut map = locks.lock().expect("publish lock map poisoned");
172+
map.entry(local_path.to_path_buf())
173+
.or_insert_with(|| Arc::new(Mutex::new(())))
174+
.clone()
175+
}
176+
159177
/// Decompress a tar.zst byte vector into a local directory.
160178
///
161179
/// Extraction is atomic with respect to `local_path`: the archive is unpacked
@@ -171,10 +189,12 @@ fn decompress_repo(data: &[u8], local_path: &Path) -> Result<()> {
171189
.file_name()
172190
.context("repo path has no file name")?
173191
.to_string_lossy();
174-
let tmp_dir = parent.join(format!(".{file_name}.tmp-extract"));
192+
// Unique per-extraction temp dir: a fixed name would let two concurrent
193+
// extractions of the same repo share one dir and clobber each other's
194+
// in-progress unpack. A fresh UUID also means it can't collide with a
195+
// leftover dir from a previously-interrupted run.
196+
let tmp_dir = parent.join(format!(".{file_name}.tmp-extract.{}", uuid::Uuid::new_v4()));
175197

176-
// Clear any leftover temp dir from a previously-interrupted extraction.
177-
let _ = std::fs::remove_dir_all(&tmp_dir);
178198
std::fs::create_dir_all(&tmp_dir).context("creating temp extract dir")?;
179199

180200
// Unpack into the temp dir; on any failure, clean up and bail without
@@ -192,7 +212,11 @@ fn decompress_repo(data: &[u8], local_path: &Path) -> Result<()> {
192212

193213
// Swap the freshly-extracted repo into place. rename within the same parent
194214
// is effectively atomic, but most platforms refuse to rename onto a
195-
// non-empty dir, so remove the old copy first.
215+
// non-empty dir, so remove the old copy first. Serialize this per repo path:
216+
// concurrent extractions unpack into isolated temp dirs, but their swaps
217+
// must not interleave or they race to a nondeterministic overwrite/failure.
218+
let lock = publish_lock(local_path);
219+
let _publish = lock.lock().expect("publish lock poisoned");
196220
if local_path.exists() {
197221
std::fs::remove_dir_all(local_path).context("removing stale repo dir")?;
198222
}

‎crates/gitlawb-node/src/rate_limit.rs‎

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -34,9 +34,17 @@ impl RateLimiter {
3434
async fn check(&self, key: &str) -> bool {
3535
let now = Instant::now();
3636
let mut state = self.state.lock().await;
37-
let window = state.entry(key.to_string()).or_insert_with(|| Window {
38-
timestamps: Vec::new(),
39-
});
37+
// Look up before inserting so the common case (key already tracked)
38+
// doesn't allocate a String per request.
39+
if !state.contains_key(key) {
40+
state.insert(
41+
key.to_string(),
42+
Window {
43+
timestamps: Vec::new(),
44+
},
45+
);
46+
}
47+
let window = state.get_mut(key).expect("window just ensured");
4048
window
4149
.timestamps
4250
.retain(|t| now.duration_since(*t) < self.window);

‎crates/gitlawb-node/src/sync.rs‎

Lines changed: 68 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -137,6 +137,17 @@ async fn run(
137137
}
138138
}
139139

140+
/// Find the origin node's base HTTP URL for a sync item, trimming any trailing
141+
/// slash so callers can append `/{path}` cleanly. Returns `None` when no peer
142+
/// row matches the item's origin DID. Kept pure (no DB) so the per-batch peer
143+
/// resolution can be unit-tested without a database.
144+
fn resolve_origin_url(peers: &[crate::db::PeerRecord], node_did: &str) -> Option<String> {
145+
peers
146+
.iter()
147+
.find(|p| p.did == node_did)
148+
.map(|p| p.http_url.trim_end_matches('/').to_string())
149+
}
150+
140151
async fn process_batch(
141152
db: &Db,
142153
config: &Config,
@@ -152,19 +163,27 @@ async fn process_batch(
152163
}
153164
};
154165

155-
for item in items {
156-
// Resolve origin node HTTP URL from peer table
157-
let peers = match db.list_peers().await {
158-
Ok(p) => p,
159-
Err(e) => {
160-
warn!(err = %e, "failed to list peers for sync");
166+
// Nothing queued — the common case on the 30s idle poll. Bail before the
167+
// peer lookup so an empty batch costs zero extra DB round-trips.
168+
if items.is_empty() {
169+
return;
170+
}
171+
172+
// Resolve the peer table once per batch — every item only needs a lookup.
173+
let peers = match db.list_peers().await {
174+
Ok(p) => p,
175+
Err(e) => {
176+
warn!(err = %e, "failed to list peers for sync");
177+
for item in &items {
161178
let _ = db.mark_sync_failed(&item.id).await;
162-
continue;
163179
}
164-
};
180+
return;
181+
}
182+
};
165183

166-
let origin_url = match peers.iter().find(|p| p.did == item.node_did) {
167-
Some(p) => p.http_url.trim_end_matches('/').to_string(),
184+
for item in items {
185+
let origin_url = match resolve_origin_url(&peers, &item.node_did) {
186+
Some(url) => url,
168187
None => {
169188
warn!(node_did = %item.node_did, repo = %item.repo, "no peer URL found for sync origin — skipping");
170189
let _ = db.mark_sync_failed(&item.id).await;
@@ -762,4 +781,43 @@ mod tests {
762781
register_replica_with_origin(&client, &keypair, Some(""), "http://127.0.0.1:1", "o", "r")
763782
.await;
764783
}
784+
785+
fn peer(did: &str, http_url: &str) -> crate::db::PeerRecord {
786+
crate::db::PeerRecord {
787+
did: did.to_string(),
788+
http_url: http_url.to_string(),
789+
last_seen: None,
790+
last_ping_ok: false,
791+
announced_at: String::new(),
792+
}
793+
}
794+
795+
#[test]
796+
fn resolve_origin_url_matches_and_trims_trailing_slash() {
797+
let peers = vec![
798+
peer("did:key:a", "https://a.example/"),
799+
peer("did:key:b", "https://b.example"),
800+
];
801+
// Trailing slash is trimmed so callers can append `/{path}` cleanly.
802+
assert_eq!(
803+
resolve_origin_url(&peers, "did:key:a").as_deref(),
804+
Some("https://a.example")
805+
);
806+
// Already-trimmed URLs pass through unchanged.
807+
assert_eq!(
808+
resolve_origin_url(&peers, "did:key:b").as_deref(),
809+
Some("https://b.example")
810+
);
811+
}
812+
813+
#[test]
814+
fn resolve_origin_url_returns_none_for_unknown_did() {
815+
let peers = vec![peer("did:key:a", "https://a.example")];
816+
assert_eq!(resolve_origin_url(&peers, "did:key:unknown"), None);
817+
}
818+
819+
#[test]
820+
fn resolve_origin_url_returns_none_for_empty_peer_list() {
821+
assert_eq!(resolve_origin_url(&[], "did:key:a"), None);
822+
}
765823
}

0 commit comments

Comments
 (0)