LooseObjects#tryMove fails with AccessDeniedException on Windows when several threads insert the same object

Summary

On Windows, inserting the same loose object concurrently from several threads fails with
ObjectWritingException: Unable to create new object, caused by a java.nio.file.AccessDeniedException
thrown out of LooseObjects#tryMove.

There is a TOCTOU window between the dst.exists() check at the top of LooseObjects#insert and the
Files.move(..., ATOMIC_MOVE) performed by tryMove:

Thread A                                     Thread B
-----------------------------------------    -----------------------------------------
dst.exists() -> false                        dst.exists() -> false
Files.move(tmpA, dst, ATOMIC_MOVE) -> ok
dst.setReadOnly()
                                             Files.move(tmpB, dst, ATOMIC_MOVE)
                                               -> dst now exists AND is read-only
                                               -> AccessDeniedException

On POSIX this race is harmless: rename(2) replaces the destination regardless of the destination's
mode bits. On Windows it is not: ATOMIC_MOVE maps to MoveFileEx(..., MOVEFILE_REPLACE_EXISTING),
which fails with ERROR_ACCESS_DENIED when the destination carries the read-only attribute -- and JGit
itself sets that attribute, one line below the move, in tryMove.

The failure is spurious. The object that the winning thread placed at dst is complete and correct
(it is necessarily byte-identical, since the object id is a hash of the content), so the losing thread
should have observed EXISTS_LOOSE rather than FAILURE. I verified this: after a failure, the
destination file is a valid, readable blob.

This is the sibling of bug 397217

Bug 397217 -- "race condition affecting
org.eclipse.jgit.storage.file.ObjectDirectoryInserter.insert(...) when called concurrently from
multiple threads" -- fixed the first race in this very code path: concurrent creation of the
fan-out objects/?? directory. It was fixed in
5dcc8693
("Fix concurrent creation of fan-out object directories") with the rationale:

All we require is that the directory does indeed exist, so not being able to create it is not
actually a fatal problem.

The identical rationale applies one step later: all we require is that the object does indeed exist.
The Files.move step was never covered by that fix.

JGit already has the Windows-aware rename helper -- tryMove just does not use it

FileUtils#rename implements precisely the workaround needed here: a retry loop gated on
FS.DETECTED.retryFailedLockFileCommit() (which FS_Win32 returns true for), plus a
delete-then-move fallback. It even carries the comment // On *nix there is no try, you do or do not.

LooseObjects#tryMove, however, calls Files.move directly and therefore gets none of it.

Steps to reproduce

Compile/run with org.eclipse.jgit, JavaEWAH and slf4j-api on the classpath
(add slf4j-simple to also see the AccessDeniedException that LooseObjects logs):

java -cp "<jgit.jar>;<JavaEWAH.jar>;<slf4j-api.jar>" JGitLooseObjectRace.java <threads> <rounds>

import java.nio.file.Files;
import java.nio.file.Path;
import java.util.ArrayList;
import java.util.List;
import java.util.concurrent.*;
import org.eclipse.jgit.storage.file.FileRepositoryBuilder;
import org.eclipse.jgit.lib.Constants;
import org.eclipse.jgit.lib.ObjectInserter;
import org.eclipse.jgit.lib.Repository;

/**
 * Several threads insert the *same* blob into the same repository at the same
 * time. On Windows this fails with ObjectWritingException, caused by
 * AccessDeniedException raised by LooseObjects#tryMove.
 */
public class JGitLooseObjectRace {
  public static void main(String[] args) throws Exception {
    int threads = args.length > 0 ? Integer.parseInt(args[0]) : 8;
    int rounds = args.length > 1 ? Integer.parseInt(args[1]) : 200;

    Path gitDir = Files.createTempDirectory("jgit-race").resolve("repo.git");
    Repository repo = new FileRepositoryBuilder().setGitDir(gitDir.toFile()).build();
    repo.create(true);

    ExecutorService pool = Executors.newFixedThreadPool(threads);
    CyclicBarrier barrier = new CyclicBarrier(threads);
    int failures = 0;
    String first = null;

    for (int r = 0; r < rounds; r++) {
      final byte[] content = ("identical blob content, round " + r + "\n").getBytes("UTF-8");
      List<Future<String>> futures = new ArrayList<>();
      for (int t = 0; t < threads; t++) {
        futures.add(pool.submit(() -> {
          try {
            barrier.await();
          } catch (Exception ignored) {
            // barrier only maximises the collision window
          }
          try (ObjectInserter ins = repo.newObjectInserter()) {
            ins.insert(Constants.OBJ_BLOB, content);
            ins.flush();
            return null;
          } catch (Throwable e) {
            return stack(e);
          }
        }));
      }
      for (Future<String> f : futures) {
        String err = f.get();
        if (err != null) {
          failures++;
          if (first == null) first = err;
        }
      }
    }
    pool.shutdown();
    repo.close();

    if (first != null) {
      System.out.println("---- first failure ----");
      System.out.println(first);
    }
    System.out.printf("jgit=%s threads=%d rounds=%d -> failed inserts: %d%n",
        Repository.class.getPackage().getImplementationVersion(),
        threads, rounds, failures);
  }

  private static String stack(Throwable e) {
    java.io.StringWriter w = new java.io.StringWriter();
    e.printStackTrace(new java.io.PrintWriter(w));
    return w.toString();
  }
}

Results

Single-threaded is always clean; anything from two threads up fails. This is not a recent regression --
7.6.0 behaves the same.

Stack traces

Root cause, as logged by LooseObjects (the LOG.error in insert):

java.nio.file.AccessDeniedException: C:\...\repo.git\objects\noz18053320105354648219.tmp -> C:\...\repo.git\objects\97\d5dbe2d2d2ccd2b1579fc894158ba36b58eeb5
	at java.base/sun.nio.fs.WindowsException.translateToIOException(WindowsException.java:89)
	at java.base/sun.nio.fs.WindowsException.rethrowAsIOException(WindowsException.java:103)
	at java.base/sun.nio.fs.WindowsFileCopy.move(WindowsFileCopy.java:310)
	at java.base/sun.nio.fs.WindowsFileSystemProvider.move(WindowsFileSystemProvider.java:287)
	at java.base/java.nio.file.Files.move(Files.java:1319)
	at org.eclipse.jgit.internal.storage.file.LooseObjects.tryMove(LooseObjects.java:342)
	at org.eclipse.jgit.internal.storage.file.LooseObjects.insert(LooseObjects.java:328)
	at org.eclipse.jgit.internal.storage.file.ObjectDirectory.insertUnpackedObject(ObjectDirectory.java:591)
	at org.eclipse.jgit.internal.storage.file.ObjectDirectoryInserter.insertOneObject(ObjectDirectoryInserter.java:130)
	at org.eclipse.jgit.internal.storage.file.ObjectDirectoryInserter.insert(ObjectDirectoryInserter.java:86)
	at org.eclipse.jgit.internal.storage.file.ObjectDirectoryInserter.insert(ObjectDirectoryInserter.java:55)
	at org.eclipse.jgit.lib.ObjectInserter.insert(ObjectInserter.java:337)

What the caller actually sees:

org.eclipse.jgit.errors.ObjectWritingException: Unable to create new object: C:\...\repo.git\objects\97\d5dbe2d2d2ccd2b1579fc894158ba36b58eeb5
	at org.eclipse.jgit.internal.storage.file.ObjectDirectoryInserter.insertOneObject(ObjectDirectoryInserter.java:142)
	at org.eclipse.jgit.internal.storage.file.ObjectDirectoryInserter.insert(ObjectDirectoryInserter.java:86)
	at org.eclipse.jgit.internal.storage.file.ObjectDirectoryInserter.insert(ObjectDirectoryInserter.java:55)
	at org.eclipse.jgit.lib.ObjectInserter.insert(ObjectInserter.java:337)

Real-world impact

Found while running FinerGit, a tool that rewrites a Git
repository into a finer-grained one (built on git-stein). It
rewrites commits in parallel -- by default with #CPU - 1 threads -- and writes a very large number of
identical blobs, because a method that does not change across commits produces the same blob every
time. Collisions are therefore constant rather than exotic.

Converting a 440-commit repository on Windows:

The whole conversion aborts, because ObjectWritingException propagates out of the worker threads.
This is the same class of workload as The BFG Repo Cleaner, which is what motivated bug 397217.

Suggested fix

In LooseObjects#insert, before treating an IOException from tryMove as FAILURE, re-check
whether the destination now exists -- if it does, another thread won the race and the correct result is
EXISTS_LOOSE, not a failure:

} catch (IOException e) {
    if (dst.isFile()) {
        // Another thread inserted the very same object concurrently. On
        // Windows the ATOMIC_MOVE onto the (now read-only) destination fails
        // with AccessDeniedException; this is not a real error.
        FileUtils.delete(tmp, FileUtils.RETRY | FileUtils.SKIP_MISSING);
        unpackedObjectCache().add(id);
        return InsertLooseObjectResult.EXISTS_LOOSE;
    }
    LOG.error(e.getMessage(), e);
    FileUtils.delete(tmp, FileUtils.RETRY | FileUtils.SKIP_MISSING);
    return InsertLooseObjectResult.FAILURE;
}

Both catch (IOException) blocks in insert need this. Alternatively, route tryMove through
FileUtils#rename, which already carries the Windows retry/fallback logic.

Environment

• JGit 7.7.1.202607240634-r (also reproduced on 7.6.0.202603022253-r)
• Windows 11 Pro 10.0.26200, NTFS, local fixed disk
• Oracle JDK 25.0.2 (build 25.0.2+10-LTS-69, HotSpot 64-Bit Server VM)
• 20 logical CPUs

Only Windows was available for testing; POSIX platforms are expected to be unaffected, for the reason
given above.