Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 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
2 changes: 1 addition & 1 deletion crates/git-cache-domain/src/materializer/direct_git.rs
Original file line number Diff line number Diff line change
Expand Up @@ -989,8 +989,8 @@ impl Materializer {

pub async fn optimize_repo_for_serving(&self, repo: &RepoKey) -> CoreResult<()> {
let repo_dir = self.ensure_repo_dir(repo).await?;
let _mutation_lock = self.lock_repo_mutation(repo).await?;
let _repo_lock = self.lock_repo(repo).await?;
let _mutation_lock = self.lock_repo_mutation(repo).await?;

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

let's make this an helper and refactor it, it has been repeated 5 times already

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Done in e9fa23b. Extracted Materializer::lock_repo_for_mutation, which acquires both locks in the canonical order and returns a RepoMutationLocks RAII guard (fields declared mutation-first so the inner mutation guard releases before the outer eviction lock). All five adjacent co-acquisition sites now call it — so the order lives in one place and a new caller can't reintroduce the inversion. The two sites that hold lock_repo across other work and take the mutation lock later (serving read-through, branch fetch) keep their separate acquisitions since they aren't a single adjacent co-acquisition. No behavior change; fmt/clippy clean, domain tests 103/0.

self.configure_served_repo(&repo_dir).await?;
self.state.git.repack_for_serving(&repo_dir).await?;
Ok(())
Expand Down
6 changes: 2 additions & 4 deletions crates/git-cache-domain/src/materializer/generations.rs
Original file line number Diff line number Diff line change
Expand Up @@ -828,10 +828,8 @@ impl Materializer {
}

let repo_dir = self.ensure_repo_dir(repo).await?;
let outer_mutation_lock = self.lock_repo_mutation(repo).await?;
let outer_repo_lock = self.lock_repo(repo).await?;
let _repo_lock = outer_repo_lock;
let _mutation_lock = outer_mutation_lock;
let _repo_lock = self.lock_repo(repo).await?;
let _mutation_lock = self.lock_repo_mutation(repo).await?;
Box::pin(self.hydrate_generation(repo, &repo_dir, head.generation)).await?;
self.state.git.repack_for_serving(&repo_dir).await?;

Expand Down
67 changes: 63 additions & 4 deletions crates/git-cache-domain/src/materializer/planning.rs
Comment thread
devin-ai-integration[bot] marked this conversation as resolved.
Original file line number Diff line number Diff line change
Expand Up @@ -742,9 +742,32 @@ impl Materializer {
.commit_ready_for_serving_no_lazy(&repo_dir, upstream_commit)
.await;
if local_tip_ready {
if self
.commit_history_complete_no_lazy(&repo_dir, upstream_commit)
.await?
// The full-history closure check below
// (`commit_history_complete_no_lazy` → `git rev-list --objects
// --missing=print` over the whole reachable graph) is O(repo
// history) — ~90s on torvalds/linux, right against the front-door
// timeout even for a warm no-op materialize. Skip it when a
// published generation already attests this commit's closure is
// complete: completeness was proven at publish time, objects are
// immutable, and the durable generation (not the disposable local
// repo) is the source of truth. The pack-presence guard inside
// `branch_tip_closure_attested` keeps it honest — only short-circuit
// when the generation's packs are actually present locally.
let attested = self
.branch_tip_closure_attested(repo, upstream_commit)
.await?;
if attested {
info!(
%repo,
%branch,
upstream_commit = %upstream_commit,
"branch tip closure attested by complete generation manifest; skipping full-history completeness walk"
);
}
if attested
|| self
.commit_history_complete_no_lazy(&repo_dir, upstream_commit)
.await?
{
self.record_verified_branch_refs(
repo,
Expand Down Expand Up @@ -980,6 +1003,42 @@ impl Materializer {
Ok(commit)
}

/// Whether a published generation already attests this commit's full
/// closure is complete + verified, letting the O(history) completeness walk
/// be skipped. True when a `complete` commit manifest exists for the commit
/// and the generation it names still exists in the object store with at
/// least one pack (i.e. has not been swept by retention, and is not a
/// legacy/empty manifest). Completeness was proven at publish time and
/// objects are immutable, so the durable generation — not the disposable
/// local repo — is the source of truth; any local gap is recovered by the
/// serving hydrate path. A missing/incomplete manifest or a swept generation
/// returns false, falling back to the full walk.
///
/// Note: this intentionally does not probe local pack files. The local repo
/// names packs by git's own hash, not the manifest's content-addressed
/// sha256, so a filename match only happens for hydrated (not freshly
/// published/fetched) packs — making such a check both wrong and unable to
/// fire. Local serveability is the serving path's concern, not materialize's.
async fn branch_tip_closure_attested(
&self,
repo: &RepoKey,
commit: &CommitSha,
) -> CoreResult<bool> {
let Some(manifest) = self.get_commit_manifest(repo, commit).await? else {
return Ok(false);
};
if !manifest.complete {
return Ok(false);
}
// Require the attesting generation to still exist and carry packs: a
// `#[serde(default)]` legacy manifest missing its `packs` field would
// otherwise vacuously attest completeness with no backing objects.
Ok(self
.get_generation_manifest(repo, manifest.generation)
.await?
.is_some_and(|generation| !generation.packs.is_empty()))
}

/// Record durable branch manifests for a commit the local repo can
/// already serve. When the commit has a complete cached manifest this
/// only writes the ref (and default) manifests; otherwise it publishes a
Expand Down Expand Up @@ -1033,8 +1092,8 @@ impl Materializer {
commit: &CommitSha,
default_branch: bool,
) -> CoreResult<()> {
let _mutation_lock = self.lock_repo_mutation(repo).await?;
let _repo_lock = self.lock_repo(repo).await?;
let _mutation_lock = self.lock_repo_mutation(repo).await?;
self.state
.git
.update_ref(
Expand Down
2 changes: 1 addition & 1 deletion crates/git-cache-domain/src/materializer/proxy_tee.rs
Original file line number Diff line number Diff line change
Expand Up @@ -260,8 +260,8 @@ impl Materializer {
let blobless = intent.filter == Some(UploadPackFilter::BlobNone);

let repo_dir = self.ensure_repo_dir(repo).await?;
let _mutation_lock = self.lock_repo_mutation(repo).await?;
let _repo_lock = self.lock_repo(repo).await?;
let _mutation_lock = self.lock_repo_mutation(repo).await?;

// A full-object pack cannot safely clear an existing blobless
// marker (other refs may still be partial); decline so the warm
Expand Down
2 changes: 1 addition & 1 deletion crates/git-cache-domain/src/materializer/repo.rs
Original file line number Diff line number Diff line change
Expand Up @@ -528,8 +528,8 @@ impl Materializer {
}

pub async fn fetch_all_refs(&self, repo: &RepoKey, repo_dir: &FsPath) -> CoreResult<()> {
let _mutation_lock = self.lock_repo_mutation(repo).await?;
let _repo_lock = self.lock_repo(repo).await?;
let _mutation_lock = self.lock_repo_mutation(repo).await?;
let remote = self.upstream_url(repo)?;
self.upstream_git(&remote)?
.fetch_all_heads(repo_dir, &remote, git_cache_git::FetchOptions::default())
Expand Down