Re: [PATCH 1/2] http: use unique tempfiles for packfile URI downloads

From: Jeff King <peff@peff.net>
Date: Jul 14, 2026, 05:28 UTC
Message-ID: <20260714052833.GA2516582@coredump.intra.peff.net>
In-Reply-To: <alWXwAGWgXSXoRJv@com-76773>

On Mon, Jul 13, 2026 at 06:58:24PM -0700, Ted Nyman wrote:

Show 11 quoted lines

> > Are there better ways for these processes to coordinate with each
> > other? Instead of appending to the file, what if the second process
> > uses a predictable temporary name (which we already use) to open a
> > new file with O_CREAT | O_EXCL to avoid this redundant work?
> 
> Using the existing pack-<hash>.pack.temp name with O_CREAT | O_EXCL
> would prevent concurrent writes, but EEXIST alone would not
> distinguish an in-progress download from one left by an earlier
> failed or interrupted invocation. The existing .pack.temp name is not
> covered by the tmp_* pruning path, so simply waiting for it to
> disappear could leave a fetch stuck after a crash.

A few thoughts:

  - Using O_EXCL makes this essentially a lockfile. So we could apply
    the logic used elsewhere for lockfiles, like auto-removing files
    with ancient mtimes. Or we could even go all-in with a pid check for
    liveness; most of Git's lockfiles don't do that, but at least one
    does (the background auto-gc lock).

  - If we're not already using a name which is auto-cleaned during
    maintenance, we probably ought to be. Leaving aside concurrency
    issues, nobody would ever clean up the on-disk cruft.

    But of course the original code here is intentionally _not_ using a
    name we'd clean up, because it wants to be able to resume an
    interrupted transfer.  And you're explicitly breaking that for the
    packfile URI case.

    Is that a cost we're OK with paying? Fixing it opens up that same
    coordination can of worms. You have to tell the difference a
    concurrent writer and a previous dead one (whose work you can
    resume).

    It does feel weird that we'd do one thing for dumb-http and another
    for packfile URIs. Wouldn't they suffer from the same concurrency
    and resumption problems?

Show 6 quoted lines

> The unique tempfile preserves the existing "download, index, then
> install" behavior for each invocation and fixes both the
> concurrent-append and EOF-resume failures. Avoiding the duplicate
> transfer would be useful for large packs, but I would prefer to keep
> that as a follow-up unless you think it is necessary for this
> correctness fix.

If we're OK with killing the ability to resume, then yeah, I think it would make sense to start simple and un-break things. And then put a coordination layer on top later (or never if nobody cares enough).

-Peff