Replace generation bundle chains with content-addressed pack storage#79
Conversation
Co-Authored-By: Şahin Olut <olut.sahin@gmail.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
⚙️ Control Options:
|
Co-Authored-By: Şahin Olut <olut.sahin@gmail.com>
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| let old_generations = self.list_generation_ids(repo).await?; | ||
| let mut old_packs: BTreeMap<String, u64> = BTreeMap::new(); | ||
| let mut old_commits = Vec::new(); | ||
| for generation in &old_generations { | ||
| let Some(manifest) = self.get_generation_manifest(repo, *generation).await? else { | ||
| continue; | ||
| }; | ||
| for pack in &manifest.packs { | ||
| old_packs.insert(pack.key.clone(), pack.len); | ||
| } | ||
| for commit in &manifest.commits { | ||
| push_unique_commit(&mut old_commits, commit.clone()); | ||
| } | ||
| } | ||
| let old_generation_set: HashSet<GenerationId> = old_generations.iter().copied().collect(); | ||
| let all_old_generation_bytes = self | ||
| .bundle_bytes_for_generations(repo, &old_generations) | ||
| .await?; | ||
| let new_generation = GenerationId::new(); | ||
| if dry_run { | ||
| let bytes_reclaimed = old_packs.values().sum(); | ||
| return Ok(Some(CompactionReport { | ||
| repo: repo.clone(), | ||
| old_chain_depth: chain.len(), | ||
| old_pack_count: head_manifest.packs.len(), | ||
| old_generations, | ||
| new_generation, | ||
| bytes_reclaimed: all_old_generation_bytes, | ||
| bytes_reclaimed, | ||
| })); | ||
| } | ||
|
|
||
| let repo_dir = self.ensure_repo_dir(repo).await?; | ||
| let _repo_lock = self.lock_repo(repo).await?; |
There was a problem hiding this comment.
🚩 Compaction reads head manifest and lists generations before acquiring repo lock
In compact_generation_chain_inner, the head manifest is read and list_generation_ids is called at crates/git-cache-domain/src/materializer/generations.rs:626-649, but the repo lock is only acquired at line 664. A concurrent publish between these lines could add a new generation whose packs reference old packs. The compaction would then delete those old packs at lines 748-753, orphaning the concurrent generation's manifest. This is mitigated by the local in-process repo lock and the fact that compaction is an hourly background job, but on multi-node deployments without distributed locking, the window exists. This is a pre-existing architectural limitation (the old chain-based compaction had the same TOCTOU pattern), not introduced by this PR.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Acknowledged — as the finding notes, this TOCTOU window between reading generation state and acquiring the repo lock is pre-existing (the chain-based compaction had the same pattern) and is mitigated by the in-process repo lock on single-writer deployments. A distributed lock / conditional-PUT on the head pointer would be the proper fix for multi-node compaction, which is out of scope for this format change; leaving as-is.
Co-Authored-By: Şahin Olut <olut.sahin@gmail.com>
| fi | ||
|
|
||
| # Must match OBJECT_STORE_SCHEMA_SUFFIX in crates/git-cache-domain/src/state.rs. | ||
| OBJECT_STORE_SCHEMA_SUFFIX="v3" |
There was a problem hiding this comment.
share this directly with state.rs - state.rs can read from a file using include_str! macro, create a separate file?
There was a problem hiding this comment.
Done in 27fecb0 — a single crates/git-cache-domain/object-store-schema-suffix file is now the source of truth: state.rs embeds it with include_str!(...).trim_ascii() and the deploy script reads it via tr -d '[:space:]' < "$REPO_ROOT/crates/git-cache-domain/object-store-schema-suffix" (failing fast if empty), so the two can no longer drift.
Preview benchmark results (PR #74 matrix rerun, linux / llvm / ruff)Tested on a live preview at PR head ( Hydration headline — the paths (c) targeted
Hydrate log:
|
| Test | #74 baseline | PR #79 | Result |
|---|---|---|---|
T1 linux blobless --depth 1 cold |
223.7s | 77.2s (re-run 82.4s) | passed |
T2 linux full --depth 1 after blobless hydration |
15.5s | 17.8s (re-run 16.3s) | passed |
| T4 llvm blobless full-history cold | 1042s / 583,851 commits | 1613s / 583,961 commits, clean status | passed with caveat: 55% over baseline (>1300s criterion); path untouched by this PR, likely upstream/network variance, but flagging |
| T5 ruff full cold / warm / shallow | 35.8 / 11.4 / 1.79s | 34.2 / 10.7 / 1.69s | passed |
Integrity per clone: non-empty worktree, git status --porcelain empty, full git log walks for full-history clones.
Both previews torn down (data prefixes retained, DELETE_DATA=false).
A blobless (--filter=blob:none) fetch makes git persist remote.<url>.partialclonefilter in the cache repo config, and git silently re-applies that saved filter to later unfiltered fetches from the same URL -- including the forced --refetch --unshallow that is supposed to fully hydrate the repo. The refetch exits 0 but the pack is still blobless, so upload-pack (GIT_NO_LAZY_FETCH) later dies with 'could not fetch <blob> from promisor remote'. Fix: when an upstream fetch has no filter, prepend -c remote.<url>.partialclonefilter= so the per-invocation option is the sole source of truth. Regression test reproduces the bug locally (fails without the fix) using a file:// upstream with uploadpack.allowFilter enabled. Co-Authored-By: Şahin Olut <olut.sahin@gmail.com>
Run the persisted-partialclonefilter regression against both LocalGitBackend implementations. Both variants fail without the fix and pass with it, confirming the bug lives in the shared subprocess upstream-fetch path rather than either backend. Co-Authored-By: Şahin Olut <olut.sahin@gmail.com>
T3 promisor-blob failure: root-caused and fixed (commits 3ed4aa9, f3d9eb0)Root cause — a blobless ( Git then silently re-applies that saved filter to every later unfiltered fetch from the same URL — including the forced Fix — when an upstream fetch has no filter, prepend Conclusive verification (independent of where linux HEAD moves): Live preview re-run on the fixed build (fully cold S3+EBS)
Post-clone checks: Deterministic local regression test (no dependence on upstream linux)
Parametrized over both CI: 18/18 green. Session: https://app.devin.ai/sessions/a5e627993756406f84d6d0b40d3a4165 |
A single crates/git-cache-domain/object-store-schema-suffix file is the source of truth: state.rs embeds it via include_str! and the deploy script reads it at runtime, so the deploy-time S3 prefix and runtime schema suffix cannot drift. Co-Authored-By: Şahin Olut <olut.sahin@gmail.com>
Co-Authored-By: Şahin Olut <olut.sahin@gmail.com>
Co-Authored-By: Şahin Olut <olut.sahin@gmail.com>
T2 regression fixed (commit 48a173d) — live preview verificationFix: before forcing the partial-hydration Live numbers (preview 48a173d, torvalds/linux, S3+EBS wiped between iterations, proxy-on-miss opted out):
All T2 clones pass integrity ( T3 sanity (regression guard): full-history clone after the blobless+shallow hydration still succeeds — 1021s, integrity OK, pinned commit Local coverage: new |
…fetch) (#131) * Make branch materialize incremental by hydrating the prior generation `ensure_branch_from_verified_tip` forced a full `--unshallow` (+ full `--refetch`) of the entire upstream history whenever the shared bare repo was shallow/blobless — which it almost always is, because the direct-git serving/deepen path leaves it that way. On a large repo this is the whole history every time: a `/v1/materialize` of `torvalds/linux master` measured **934s (~15.6m)** against the origin, and it's why the durable generation goes stale (the only refresh path is this heavy, Cloudflare-timeout-prone operation). The incremental publish machinery already exists (#79: `publish_generation` packs `--revs <new> --not <previous_tips>` when a parent generation exists). What blocked it was the fetch side re-pulling everything. Fix: when the repo is shallow and the object store holds a complete prior generation, hydrate that generation (fast parallel content-addressed pack download) and drop the now-stale shallow boundary, so the upstream fetch transfers only the delta to the new tip and the publish packs only the new commits. Safety: a published generation is a verified full-history closure, so hydrating it makes the prior history genuinely complete and removing the `shallow` file is correct. If hydration is unavailable (no prior generation — e.g. the first/base materialize) or still leaves the closure incomplete, the existing `commit_history_complete_no_lazy` guard falls back to the full unshallow/refetch path. So the change can only save time on the happy path; it can never publish an incomplete generation. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * Include the depth+deepen prod matrix tooling Bundles the extended scripts/aws/test-prod-linux-cache-depths.sh (multiple depth orders incl. 1 50 10, a fetch --deepen phase, a 5xx/exit failure gate, RUN_METRICS toggle, and a stale-`*.lock` hot-cache inspect) into this PR, since it's the matrix used to regression-verify this change against a preview. This supersedes the standalone PR #127. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
1 participant
Summary
Implements item (d) from
docs/clean-slate-design.md(merged in #75) — content-addressed packs replacing generation chains: the durable S3 layer no longer stores ordered generation bundle chains — it stores content-addressed packs plus self-contained generation manifests. Clean format break (OBJECT_STORE_SCHEMA_SUFFIXbumpedv2→v3, no migration by design).New durable layout
What changed per flow
git pack-objects --revsemits (nogit bundle create), skipping packs already in S3 by address; an incremental generation manifest references the previous generation's packs plus the new one.GenerationPublish::publish_pack_filesreplaces the pending-bundle publish.JoinSet+Semaphore(4)),git index-packeach, then one atomicgit update-ref --stdinbatch — replaces sequential bundle-chain replay.sha256of bytes == key), so generations publish already-verified; the entire pending-publish/verification machinery (verified.json, pending scan keys, verification semaphore) is deleted.chain_depth_threshold(name kept; semantics now = pack count), repack into a single pack, publish a single-pack generation, repoint commit/ref manifests, delete old generation manifests, and GC unreferenced packs.CompactionReport.old_chain_depth→old_pack_count.Bugfix: blobless hydration silently re-filtered later full refetches
A blobless read-through fetch makes git persist
remote.<url>.partialclonefilter = blob:nonein the cache repo config; git silently re-applies that saved filter to the later forced unfiltered--refetch --unshallow, leaving blobs missing while exiting 0 — upload-pack then died withcould not fetch <blob> from promisor remote(live T3 linux failure; latent since #74, exposed by upstream state). Fix: unfiltered upstream fetches prepend-c remote.<url>.partialclonefilter=so the per-invocation filter option is the sole source of truth. Regression test parametrized over both gix and git-subprocess backends; verified live (linux T3: exit 0, 1,447,120 commits — see PR comments for the benchmark matrix and fix verification).Perf fix: depth-1 completeness probe skips unnecessary forced refetch
The correctness fix above made every full clone after a blobless hydration re-download the depth-1 pack from upstream (T2: 46-49s vs 15.5s baseline), even though the client's checkout lazy-fetches had already filled the cache. Depth-1 intents now run a local no-lazy-fetch completeness probe (
rev-list --objects --missing=print --max-count=1per want); if every want's snapshot is fully present, the forced refetch is skipped and the pack is served from cache. Verified live: T2 back to 14.8-15.0s with T3 still passing.Config/env names (
chain_depth_threshold,GIT_CACHE_COMPACTION_CHAIN_DEPTH_THRESHOLD) and deploy scripts are unchanged; docs and the runtime-testing skill are updated to the new semantics.Note: this will conflict with #76 (bundle-uri, design-doc item (c)) — the bundle-uri base-bundle publishing will need rebasing onto the pack layout once one of the two merges.
Link to Devin session: https://app.devin.ai/sessions/a5e627993756406f84d6d0b40d3a4165
Requested by: @0lut