MultiPackIndex.find() hands out shared mutable state — concurrent lookups return wrong object offsets

[Jallibad]

Version

7.7.1.202607240634-r

Operating System

MacOS, Linux/Unix

Bug description

Summary

When a repository has a multi-pack-index, MultiPackIndex.find() returns a reference to a
single PackOffset instance that it mutates in place. PackMidx.MidxPackIndex.findOffset()
reads that instance after the call returns, without copying. Two threads looking up different
objects therefore race: one can compute an offset belonging to the other's object.

Impact

ObjectDirectory is documented and widely treated as thread-safe, and is used concurrently by
multi-threaded consumers. core.multiPackIndex defaults to true in JGit, and
PackDirectory.scanPacksImpl() removes every midx-covered pack from the pack list and replaces
it with the PackMidx instance:

if (useMidx && theMidx != null) {
    // Replace the covered packs with the midx in the list
    ...
    list.add(theMidx);
}

Why this is a JGit issue and not a caller issue

The reuse contract on find() is addressed to its caller, which is JGit's own
PackMidx.MidxPackIndex.findOffset() in org.eclipse.jgit.internal.storage.file. Downstream
consumers never see MultiPackIndex; they use ObjectReader / TreeWalk.

The contract is also about lifetime, not concurrency — "callers must create a #copy() if they
want to keep a reference". findOffset() does not keep a reference; it reads the fields
immediately. That is correct single-threaded and still unsafe concurrently, so honouring the
documented contract is not sufficient to make this correct.

Meanwhile Repository documents the opposite guarantee for exactly this class:

// Repository.java
/**
 * The thread-safety of a Repository very much depends on the concrete implementation...
 * users of a Repository type must not assume the instance is thread-safe.
 * - FileRepository is thread-safe.
 * - DfsRepository thread-safety is determined by its subclass.
 */

and ObjectReader is documented as per-thread:

// ObjectReader.java
/** Reads an ObjectDatabase for a single thread. */

A consumer that shares one FileRepository across threads and takes a fresh ObjectReader per
operation is therefore using the documented API correctly. new TreeWalk(Repository) does exactly
that internally:

public TreeWalk(Repository repo) {
    this(repo, repo.newObjectReader(), true);
}

So the midx path introduces a non-thread-safe component beneath a type that promises
thread-safety, without changing that promise.

Root cause

MultiPackIndexV1 keeps one PackOffset and returns it from every call:

// MultiPackIndexV1.java
private final PackOffset result = new PackOffset();

@Override
public PackOffset find(AnyObjectId objectId) {
    int position = idx.findMultiPackIndexPosition(objectId);
    if (position == -1) {
        return null;
    }
    offsets.getObjectOffset(position, result);   // mutates the shared instance
    return result;                                // returns a reference to it
}

This is intentional and documented on the interface:

// MultiPackIndex.java
/**
 * The returned object can be reused by the implementations. Callers must
 * create a #copy() if they want to keep a reference.
 */
PackOffset find(AnyObjectId objectId);

The caller does not copy, and reads the fields after find() has returned:

// PackMidx.MidxPackIndex
@Override
public long findOffset(AnyObjectId objId) {
    MultiPackIndex.PackOffset packOffset = midx.find(objId);
    return offsetCalculator.encode(packOffset);   // reads getPackId() / getOffset()
}

Thread A's values can be overwritten by thread B between the write in find() and the reads in
encode(), yielding accSizes[B.packId] + B.offset for A's object id.

PackMidx.OffsetCalculator has the same pattern independently, on the read path:

private final MultiPackIndex.PackOffset mutablePo = new MultiPackIndex.PackOffset();

MultiPackIndex.PackOffset decode(long totalOffset) {
    ...
    return mutablePo.setValues(i, totalOffset - accSizes[i]);
}

decode() is consumed by load(), read(), mmap() and findObjectForOffset(), each of
which reads po.getPackId() / po.getOffset() after the call returns.

Actual behavior

Repository state: two packs with a multi-pack-index covering them.

git init repo && cd repo
git remote add origin <large repo>
git fetch --no-tags origin <some older ref>:refs/remotes/origin/main
git multi-pack-index write        # midx covers the big pack
git fetch --no-tags origin main   # a second, uncovered pack

Then look up known object ids concurrently through the midx-backed PackIndex, comparing
against single-threaded ground truth:

FileRepository repo = new FileRepository(gitDir);
ObjectDirectory od = (ObjectDirectory) repo.getObjectDatabase();
Pack packMidx = od.getPacks().stream()
    .filter(p -> p.getClass().getSimpleName().equals("PackMidx"))
    .findFirst().orElseThrow();
PackIndex idx = packMidx.getIndex();

// ground truth, single-threaded
long[] expected = new long[ids.size()];
for (int i = 0; i < ids.size(); i++) {
    expected[i] = idx.findOffset(ids.get(i));
}

// N threads, same PackIndex instance
for (int t = 0; t < threads; t++) {
    pool.submit(() -> {
        for (int n = 0; n < iterations; n++) {
            int i = n % ids.size();
            long got = idx.findOffset(ids.get(i));
            if (got != expected[i]) {
                wrong.incrementAndGet();   // records id, expected, got
            }
        }
    });
}

Results

512 object ids, 300,000 lookups per thread:

threads   lookups     wrong offsets
1         200,000     0
2         599,976     24
4       1,199,276     724
16      4,796,592     3,408

The returned values are not random corruption — they are other objects' valid offsets:

id 0715b34840cc20d1f6e82c395446fc5d536529aa  expected 52624527  got 52668839

where 52668839 is the correct offset of a different object looked up concurrently.

Setting core.multiPackIndex=false removes PackMidx from the pack list and the anomalies
disappear (4,800,000 concurrent lookups, zero wrong).

Note: the race is hard to observe through the full ObjectReader.open() path — 1.28M concurrent
reads produced no failures, because object loading dominates the few-instruction window. Testing
findOffset directly is what makes it reproducible.

Expected behavior

Concurrent lookups on a thread-safe ObjectDatabase return correct offsets, or the midx path is
documented as requiring external synchronisation.

Relevant log output

org.eclipse.jgit.errors.MissingObjectException: Missing tree <sha>
    at org.eclipse.jgit.internal.storage.file.WindowCursor.open(WindowCursor.java:154)
    at org.eclipse.jgit.treewalk.CanonicalTreeParser.reset(CanonicalTreeParser.java:191)
    at org.eclipse.jgit.treewalk.TreeWalk.enterSubtree(TreeWalk.java:1371)
    at org.eclipse.jgit.treewalk.TreeWalk.next(TreeWalk.java:925)

Other information

This has bitten us using Spotless with a Git ratchet. Occasionally CI flakes with a MissingObjectException. We're setting core.multiPackIndex to false as a workaround.