fix(server): compact repo checkouts in place (NoTags + reachability repack), self-heal broken clones - #260
Merged
Merged
Conversation
…broken clones Context: production hosts ~70 actively-pushed external repos; the data dir grew to ~76 GB against ~2.3 GB of database. Root cause: the update path in repocloner is fetch(Depth:1, Force) + hard reset, go-git writes ONE NEW PACKFILE per fetch, the reset makes the previously fetched tree unreachable, and nothing ever runs gc (go-git has none; the distroless runtime has no git binary to shell out to). So every push of every repo permanently added a pack, forever. Full brief with measurements: loadtests/GIT_STORAGE_CONTEXT.md (local, gitignored). Changes in repocloner.CloneOrFetch, reuse path restructured into cloneFresh/updateExisting/reclone: 1. Packfile budget (maxFetchPacks=20): before reusing a checkout, count .git/objects/pack/*.pack; at/over budget, discard the directory and shallow-clone fresh. A fresh clone is one pack; real git auto-gcs at 50. This both bounds future growth and automatically reclaims the existing bloat in production: each over-budget repo re-clones on its next webhook/poll fetch. After a re-clone PrevIndexedSHA is unreachable, so Changes=nil and the caller lands in reconcile mode (hash-gated, cheap). 2. Self-healing reuse path: failures rooted in on-disk state (PlainOpen, pre-fetch Head — the signature of a SIGKILL-mid-clone half-write that previously wedged the repo in a forever-failing error loop, remote URL mismatch after a github_url change, post-fetch ref resolve, worktree, reset) are wrapped with an errLocalState sentinel and recovered by nuke + re-clone. Fetch/transport errors deliberately stay fatal-but- preserving: a network blip must not cost a healthy clone and force a reindex. A cancelled context also never triggers the nuke. 3. Result.RecloneReason (informational) + a log line in repojobs.handleClone so operators can see why a fetch turned into a full clone. 4. maintenance.DirSizeBytes now skips unreadable subtrees and keeps counting instead of returning (0,false) on any walk error — previously one bad directory made the whole "Cloned repositories" row vanish from the Resources screen. (0,false) is still returned for a missing/unreadable root (keeps "unreadable" vs "empty" distinguishable) and on context cancellation (partial numbers from an aborted request are not cached). Not addressed here (possible follow-ups): a maintenance category for bloat inside live checkouts (the packfile budget makes it mostly redundant for actively-pushed repos, but idle bloated repos only shrink on their next fetch); per-branch checkout duplication (no shared object store across branches of the same repo); accounting for a moved CIX_REPOS_DIR stranding the old tree. Tests: 4 new repocloner tests (half-written clone self-heals; changed remote URL re-clones; packfile budget re-clones and resets the count; fetch failure against a dead upstream preserves the local clone) + 4 new DirSizeBytes tests (sum, missing root, unreadable subtree partial, cancelled context). Full server suite green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…cing budget-reclone Replaces the maxFetchPacks nuke-and-reclone bound (previous commit) with the approach validated by the PoC on branch poc/gc-compaction (server/cmd/gc-poc, run against all 45 loadtests fixtures — spring-boot, grafana, etc — with byte-level identity vs canonical full clones, git fsck --strict, a 12-round leak test, and O() scaling measurements; see that branch's commit messages for the numbers). Two production changes: 1. Tags:NoTags on both clone and fetch (repocloner.go). go-git's clone default is AllTags, and on a shallow clone every tag arrives as a FULL tree snapshot: real checkouts measured 2.6-5x the worktree on day zero (spring-boot: 391 tags -> 102MB store / 39MB worktree; worst fixture: 1240 tags, 503MB / 278MB). cix indexes exactly one branch and never reads tags. This kills the second bloat driver at the source. 2. In-place compaction (compact.go), run from CloneOrFetch after every successful update when needsCompaction() says so: packfileCount >= compactPackThreshold (4) — the fetch+reset path persists one snapshot-sized pack per fetch and go-git has no gc — OR tag refs present. The tag trigger IS the upgrade migration: the first ordinary update of every pre-NoTags checkout drops refs/tags/*, collapses the store to one branch-snapshot pack and rewrites .git/shallow, with no separate migration code. Runs on the NoChanges path too, so cleanup does not wait for the repo's next commit. Mechanism: reachability walk from non-tag refs + explicitly protected commits (PrevIndexedSHA — the base of the next incremental tree-diff, unreferenced once the branch moves, kept alive across compactions), honouring .git/shallow graft points exactly like git; encode the set into one pack via storer.PackfileWriter (window 0 — measured -17% size for 2.9x CPU with deltas, not worth it); delete old packs only after the new pack is durable (crash mid-compaction leaves extra packs, never a broken store); drop all loose objects; keep only still-present shallow entries. The walker is go-git's objectWalker with three fixes real repos hit immediately: shallow grafts terminate parent walks, submodule gitlinks are skipped, symlink-reached blobs are leaves. Costs (measured): linear time ~0.2-1.4ms CPU per object + ~0.2s/GB emitted; heap ~3x the uncompressed snapshot (go-git's encoder materialises content) — package-level compactMu serialises compactions across concurrent clone jobs so those peaks never stack on the 8GB prod hosts. Failed compaction falls back to nuke+reclone (store state unknown; shallow reclone is always correct). Result gains Compaction *CompactStats; repojobs logs it (bytes before/ after, packs deleted, tag refs dropped, duration) next to the existing RecloneReason log. Tests: fresh clone carries zero tag refs; the upgrade scenario end-to-end (legacy AllTags clone + 2 legacy fetch cycles -> first new-code update compacts: 2 tag refs dropped, packs 3+ -> 1, objects dir shrinks, the SAME update still returns the v4->v5 incremental ChangeSet, a later update still diffs from the protected-but-unreferenced indexed_sha, and a no-op cycle stays quiet); pack count stays <= threshold across 8 push/fetch cycles with at least one compaction. Budget-reclone test removed with the mechanism. Full server suite + vet green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
dvcdsys
changed the title
Review (adversarial, 8 CONFIRMED + 2 PLAUSIBLE) accepted in full; every
finding fixed, plus the runner-ups. Mapping:
1. Failed compaction no longer nukes the checkout. Compaction moved out of
CloneOrFetch entirely into exported MaybeCompact; repojobs calls it after
the clone section, logs a Warn on error and moves on — the store is
exactly what the update left (valid), and the trigger re-fires next
cycle. Nuke+reclone is reserved for the errLocalState taxonomy.
2. No more global-mutex-inside-repo-lock convoy. MaybeCompact acquires the
global compaction gate FIRST, then takes the per-repo write lock (passed
in by repojobs as a closure) only around the store mutation. A job queued
behind another repo's compaction now holds no locks at all, so readers
of its repo proceed; upgrade-day fleets serialise on the gate without
stalling each other's reads.
3. NoChanges no longer trusts HEAD: updateExisting performs the hard reset
on the NoChanges path too (a no-op write on a clean tree), repairing the
torn-worktree state a crash mid-reset leaves behind (go-git writes HEAD
before touching files). Test: NoChangesStillRepairsWorktree.
4. Crash-safe trigger ordering: tag refs are now dropped AFTER the new pack
is durable and BEFORE old packs are deleted (a ref never dangles over a
missing object). The remaining window — tags dropped, old packs still
present — is re-armed by a new needsCompaction backstop: store >= 2x
worktree (>=2 packs, 1MB floor). Test: RatioBackstopRearms simulates
exactly the crashed state.
5. compactCheckout takes ctx: checked between phases and every 1024 objects
inside the walk; cancellation aborts cleanly before any deletion.
MaybeCompact also re-checks after waiting on the gate. Test:
CancelledContext.
6. Pending index target protected: Result gains PrevHeadSHA (the on-disk
HEAD before the fetch — the TargetSHA of a possibly still-queued index
job) and repojobs passes {IndexedSHA, PrevHeadSHA} to the protect set.
Test: ProtectsPendingIndexTarget reproduces the two-cycle race.
7. Protect probe distinguishes not-found from store errors:
errors.Is(ErrObjectNotFound) -> skip; anything else -> loud failure.
Same fix applied to the walker's parent probe.
8. errLocalState gets one retry before the nuke: transient EMFILE/EIO-class
failures heal on the second attempt, a genuinely broken checkout fails
identically and still recloses. (Existing half-written-clone test covers
the broken path through the retry.)
9. DirSizeBytes undercounts are now visible: dirSizeDetail returns a
skipped-entry count, DiskUsage gains partial:true (openapi.yaml +
openapi-gen regenerated; maintenance.Usage serialises straight to the
wire) and computeUsage logs a Warn naming the disk and skipped count.
10. Reachability walk converted from recursion to an iterative worklist —
stack depth no longer scales with commit-chain length, so a full-history
clone seeded into the repos dir cannot blow the goroutine stack.
Runner-ups: the seven errLocalState wraps use double-%w so the cause chain
survives errors.Is/As; refs are read in ONE pass (tags collected and roots
seeded together); needsCompaction's ratio walk is gated on pack count >= 2
so the steady state never pays it.
Not taken (documented deliberately): clone-into-temp+rename for reclone —
reclone is now confined to genuinely-unusable-state paths where the old
checkout has no value; the review marked it optional.
Full server suite, vet, and openapi-gen sync green.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Production hosts ~70 actively-pushed external repos; the cix data directory grew to ~76 GB against ~2.3 GB of database. Investigation (PoC on branch
poc/gc-compaction, validated against 45 real fixture checkouts) found two independent bloat drivers:fetch(Depth:1, Force)+ hard reset: go-git persists one pack per fetch — and each such pack is a near-full snapshot, not a delta — while the reset makes the previous snapshot unreachable. go-git has no gc; the distroless runtime has nogitbinary. Growth is unbounded.Tags:AllTags, and on a shallow clone every tag arrives as a full tree snapshot: spring-boot (391 tags) = 102 MB store for a 39 MB worktree; worst fixture (1240 tags) = 503 MB for 278 MB — 2.6–5× the worktree on day zero. cix indexes exactly one branch and never reads tags.Plus two wedge bugs: a SIGKILL mid-clone leaves a half-written
.gitthat fails every retry forever, and a changedgithub_urlerrors permanently instead of re-cloning.Fix
Tags: NoTagson clone and fetch — kills driver 2 at the source.compact.go) via exportedMaybeCompact, called by repojobs after the write-locked clone section: the global compaction gate (which serialises the ~3×-snapshot transient heap across workers) is acquired before the per-repo write lock, so a queued compaction never stalls another repo's readers. Triggers: ≥ 4 packfiles, tag refs present (= the implicit upgrade migration: the first ordinary update of every pre-NoTags checkout cleans it, including on the NoChanges path), or a store ≥ 2× worktree ratio backstop that re-arms cleanup after a crash mid-compaction. Crash-safe ordering: new pack durable → tag refs dropped → old packs deleted → loose pruned →.git/shallowrewritten; ctx honoured between phases and inside the walk (iterative worklist, no recursion). Protect set =indexed_sha+ pre-fetch HEAD (the target of a possibly still-queued index job), so incremental diffs survive compaction in both the normal and the racing case. A failed compaction keeps the checkout — logged, re-triggered next cycle; it is never grounds for a re-clone.PlainOpen, broken HEAD, remote URL mismatch, reset) are retried once (transient EMFILE/EIO-class pressure must not cost a multi-GB checkout) and only then nuke + re-clone; fetch/transport failures always preserve the clone; cancelled contexts never trigger the nuke. The NoChanges shortcut now still runs the hard reset, repairing the torn-worktree state a crash mid-reset leaves behind.maintenance.DirSizeBytesreturns partial sums instead of(0,false)on walk errors, and undercounts are now marked:DiskUsage.partialon the wire (openapi regenerated) + a server Warn log naming the skipped entries.Validation
PoC (branch
poc/gc-compaction) against copies of all 45 loadtests fixtures: worktree byte-identity,git ls-tree -r HEADidentity,git fsck --strict— 45/45 clean; canonical comparison vs full-history clones — identical; 12-round leak test — heap/fd flat. Complexity: time linear (~0.2–1.4 ms CPU/object + ~0.2 s/GB), memory linear in snapshot size (~3×). Headline reclaim: worst fixture 503 MB → 55.7 MB (−89 %) in 9.5 s.An adversarial review of the implementation produced 10 findings (failure handling, locking, crash windows); all are fixed in 942d651 — see that commit's message for the finding-by-finding mapping.
Tests
Upgrade scenario end-to-end (NoChanges-cycle cleanup, incremental diff across compaction, protected-but-unreferenced diff base); pending-index-target race; torn-worktree repair on NoChanges; failed compaction keeps checkout + trigger armed; cancelled-context no-op; ratio-backstop re-arm on the crashed-compaction state; pack bound over 8 cycles; NoTags fresh clone; self-heal + fetch-failure-preserves-clone; DirSizeBytes ×4 incl. skipped-count. Full server suite,
go vet,openapi-gensync green.🤖 Generated with Claude Code