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); + }); +});