From cb64c59df4a6e599d0a0ed5c92eebc95c8a7eadd Mon Sep 17 00:00:00 2001 From: Chad Miller Date: Tue, 18 Aug 2026 23:16:09 -0700 Subject: [PATCH] fix(git): keep the bundle chain disjoint when pushing A push bundle's basis covered only the remote tips present locally. A tip absent from the pusher's clone was dropped, so pushing a new branch from a stale working copy re-bundled shared history; and history that only a superseded head reaches (a force push, a deleted branch) was never excluded at all. Either way the chain repeated objects, and the smart HTTP merge then served a pack a clone rejects: index-pack runs --check-self-contained-and-connected and dies on "the same object appears twice in the pack". A plain fetch tolerates the repeat, which is why only clones broke. The basis now covers the current tips plus every recorded bundle head, and the helper replays the chain to obtain any it lacks locally, so each new bundle is disjoint from the whole chain. A chain that already repeats an object still needs one repack to serve clones. Co-Authored-By: Claude Fable 5 --- .changeset/chained-bundles-stay-disjoint.md | 22 ++++ packages/git/src/helper.js | 51 +++++++-- packages/git/src/pack.js | 9 +- packages/git/test/git-remote.test.js | 118 +++++++++++++++++++- 4 files changed, 182 insertions(+), 18 deletions(-) create mode 100644 .changeset/chained-bundles-stay-disjoint.md diff --git a/.changeset/chained-bundles-stay-disjoint.md b/.changeset/chained-bundles-stay-disjoint.md new file mode 100644 index 0000000..b87e89b --- /dev/null +++ b/.changeset/chained-bundles-stay-disjoint.md @@ -0,0 +1,22 @@ +--- +'@pdsjs/git': patch +--- + +Pushes keep the bundle chain free of repeated objects. The smart HTTP +endpoint merges the chain's packfiles byte-for-byte, and a chain that +repeats an object produces a pack a clone rejects: index-pack runs with +`--check-self-contained-and-connected` and dies on "the same object +appears twice in the pack". A plain fetch tolerates the repeat, which is +why only clones broke. + +Two pushes could repeat an object. A remote tip absent from the pusher's +repository was silently dropped from the bundle basis, so pushing a new +branch from a stale working copy re-bundled shared history. And history +that only a superseded head reaches (after a force push or a branch +delete) was never excluded, so a ref re-advertising those objects bundled +them again. The basis now covers the current tips plus every recorded +bundle head, and the helper replays the chain to obtain any it lacks +locally, so each new bundle is disjoint from the whole chain. + +A chain that already repeats an object needs one repack: push any change +with `ATPROTO_GIT_REPACK_THRESHOLD=2` set. diff --git a/packages/git/src/helper.js b/packages/git/src/helper.js index 2e29c95..ef5d2f7 100644 --- a/packages/git/src/helper.js +++ b/packages/git/src/helper.js @@ -496,6 +496,23 @@ export class AtprotoRemoteHelper { if (!stored && sources.length === 0) { throw new Error(`no git repo record "${this.repoName}" to fetch from`); } + await this.replayChains(); + for (const sha of wanted) { + if (!objectExists(sha)) { + throw new Error( + `object ${sha} not present after replaying the bundle chains`, + ); + } + } + } + + /** + * Replay every chain's bundles into the local object database. Bundles + * whose heads are all present are skipped. + */ + async replayChains() { + const stored = await this.loadRecord(false); + const sources = this.space ? await this.loadMemberSources() : []; const { location, client } = await this.ensureClient(); const tmp = mkdtempSync(join(tmpdir(), 'atproto-git-')); try { @@ -549,13 +566,6 @@ export class AtprotoRemoteHelper { } finally { rmSync(tmp, { recursive: true, force: true }); } - for (const sha of wanted) { - if (!objectExists(sha)) { - throw new Error( - `object ${sha} not present after replaying the bundle chains`, - ); - } - } } /** @@ -765,13 +775,30 @@ export class AtprotoRemoteHelper { if (updates.length === 0 && deletions.length === 0) return; - // The bundle excludes history already on the remote: every remote tip - // we have locally becomes a prerequisite. - const basis = [...new Set(remoteRefs.values())].filter((sha) => - objectExists(sha), - ); /** @type {import('./record.js').BundleEntry[]} */ let bundles = stored ? [...stored.record.bundles] : []; + // The basis must cover every object already in the chain; a repeated + // object makes the smart HTTP merge (pack.js) serve a pack a clone + // rejects. Tips alone miss history that only a superseded head + // reaches. Each bundle's objects are reachable from its recorded + // heads, so the tips plus all recorded heads cover the chain. A chain + // replay supplies any basis object missing locally. + let basis = [ + ...new Set([ + ...remoteRefs.values(), + ...bundles.flatMap((bundle) => bundle.heads), + ]), + ]; + if (updates.length > 0 && !basis.every((sha) => objectExists(sha))) { + await this.replayChains(); + const missing = basis.filter((sha) => !objectExists(sha)); + if (missing.length > 0) { + this.log( + `warning: ${missing.length} basis object(s) unavailable; the new bundle may repeat chain objects until a repack\n`, + ); + basis = basis.filter((sha) => objectExists(sha)); + } + } if (updates.length > 0) { const entry = await this.buildAndUploadBundle( client, diff --git a/packages/git/src/pack.js b/packages/git/src/pack.js index f95b131..2474a3e 100644 --- a/packages/git/src/pack.js +++ b/packages/git/src/pack.js @@ -7,10 +7,11 @@ * and in order. Merging is therefore header surgery: sum the counts, * concatenate the entry regions, recompute the trailer. * - * Chained bundles are disjoint in normal histories (each push excludes - * objects reachable from the refs before it), so the merged pack has no - * duplicate objects; a repack collapses the chain if an unusual history - * ever produces one. + * A merged pack must not repeat an object: a clone runs index-pack with + * --check-self-contained-and-connected, which dies on "the same object + * appears twice in the pack". git-remote-atproto keeps chained bundles + * disjoint by excluding every recorded bundle head from each push bundle. + * A chain written without that rule needs a repack to serve clones. */ import { Sha1 } from './sha1.js'; diff --git a/packages/git/test/git-remote.test.js b/packages/git/test/git-remote.test.js index 519f879..e14d08f 100644 --- a/packages/git/test/git-remote.test.js +++ b/packages/git/test/git-remote.test.js @@ -25,7 +25,10 @@ import { defineLexicon } from '@bigmoves/lexicon'; import { LexiconResolver } from '@pdsjs/lexicon-resolver'; import { createServer } from '@pdsjs/node'; import { afterAll, beforeAll, describe, expect, it } from 'vitest'; +import { parseBundle } from '../src/bundle.js'; import { GIT_REPO_COLLECTION, gitRepoLexicon } from '../src/index.js'; +import { mergePacks } from '../src/pack.js'; +import { concatChunks } from '../src/record.js'; const execFileAsync = promisify(execFile); @@ -55,12 +58,13 @@ let gitEnv = {}; /** * @param {string} cwd * @param {string[]} args + * @param {Record} [env] - overrides merged over the shared env * @returns {Promise} stdout */ -async function runGit(cwd, args) { +async function runGit(cwd, args, env = {}) { const { stdout } = await execFileAsync('git', args, { cwd, - env: gitEnv, + env: { ...gitEnv, ...env }, encoding: 'utf8', }); return stdout; @@ -271,3 +275,113 @@ describe('git-remote-atproto e2e', () => { expect(record.refs.map((r) => r.name)).not.toContain('refs/heads/feature'); }); }); + +describe('bundle chain stays disjoint', () => { + // The repack path would collapse the chain and hide a repeated object. + const NO_REPACK = { ATPROTO_GIT_REPACK_THRESHOLD: '99' }; + /** @type {string} */ + let repoDir = ''; + /** @type {string} */ + let staleDir = ''; + /** @type {string} */ + let secondSha = ''; + let verifyCount = 0; + + /** + * Merge the chain's packfiles the way the smart HTTP endpoint does and + * have real git index the result the way a clone does. A chain that + * repeats an object fails here: "The same object appears twice in the + * pack". A plain fetch tolerates the repeat, so the clone flag matters. + * @param {string} rkey + * @returns {Promise} + */ + async function indexMergedChain(rkey) { + const record = await fetchRepoRecord(rkey); + /** @type {Uint8Array[]} */ + const packs = []; + for (const bundle of record.bundles) { + /** @type {Uint8Array[]} */ + const chunks = []; + for (const part of bundle.parts) { + const params = new URLSearchParams({ did: DID, cid: part.ref.$link }); + const res = await fetch( + `${BASE}/xrpc/com.atproto.sync.getBlob?${params}`, + ); + expect(res.ok).toBe(true); + chunks.push(new Uint8Array(await res.arrayBuffer())); + } + packs.push(parseBundle(concatChunks(chunks)).pack); + } + verifyCount += 1; + const scratch = join(workDir, `verify-${rkey}-${verifyCount}`); + mkdirSync(scratch); + await runGit(scratch, ['init', '-b', 'main']); + const packPath = join(scratch, 'merged.pack'); + writeFileSync(packPath, mergePacks(packs)); + await runGit(scratch, [ + 'index-pack', + '--check-self-contained-and-connected', + packPath, + ]); + return record; + } + + it('completes the push basis from the chain when a tip is absent locally', async () => { + repoDir = join(workDir, 'disjoint-src'); + mkdirSync(repoDir); + await runGit(repoDir, ['init', '-b', 'main']); + writeFileSync(join(repoDir, 'a.txt'), 'one\n'); + await runGit(repoDir, ['add', '.']); + await runGit(repoDir, ['commit', '-m', 'one']); + await runGit(repoDir, [ + 'remote', + 'add', + 'origin', + `atproto://${DID}/disjoint`, + ]); + await runGit(repoDir, ['push', 'origin', 'main'], NO_REPACK); + + // A working copy taken now lacks the tip the next push records. + staleDir = join(workDir, 'disjoint-stale'); + await runGit(workDir, ['clone', repoDir, staleDir]); + + writeFileSync(join(repoDir, 'b.txt'), 'two\n'); + await runGit(repoDir, ['add', '.']); + await runGit(repoDir, ['commit', '-m', 'two']); + secondSha = (await runGit(repoDir, ['rev-parse', 'main'])).trim(); + await runGit(repoDir, ['push', 'origin', 'main'], NO_REPACK); + + // Without the remote tip, a basis of local-only tips would re-bundle + // the shared history and repeat the first bundle's objects. + await runGit(staleDir, [ + 'remote', + 'add', + 'atproto', + `atproto://${DID}/disjoint`, + ]); + await runGit(staleDir, ['checkout', '-b', 'feature']); + writeFileSync(join(staleDir, 'c.txt'), 'three\n'); + await runGit(staleDir, ['add', '.']); + await runGit(staleDir, ['commit', '-m', 'three']); + await runGit(staleDir, ['push', 'atproto', 'feature'], NO_REPACK); + + const record = await indexMergedChain('disjoint'); + expect(record.bundles.length).toBe(3); + }); + + it('excludes history that only a superseded head reaches', async () => { + // Rewind main. The second commit stays in the chain with no tip on it. + await runGit(repoDir, ['reset', '--hard', 'HEAD~1']); + await runGit(repoDir, ['push', '--force', 'origin', 'main'], NO_REPACK); + + // A branch on the rewound commit re-advertises objects the chain holds, + // so the push updates refs without a new bundle. + await runGit(repoDir, ['branch', 'keep', secondSha]); + await runGit(repoDir, ['push', 'origin', 'keep'], NO_REPACK); + + const record = await indexMergedChain('disjoint'); + expect(record.bundles.length).toBe(3); + const keep = record.refs.find((r) => r.name === 'refs/heads/keep'); + expect(keep?.sha).toBe(secondSha); + }); +}); -- 2.51.2