Re: [PATCH v2 3/4] packfile: recover object lookups racing a concurrent repack

On Thu, Aug 27, 2026 at 03:23:30PM -0700, Elijah Newren wrote:

It's far more likely that I am the one being dense.  My rough line of thinking:

* We see "packfile ... index unavailable" in our logging
* There's only one thing that remove packfiles
* Investigate the mechanism
* Look for other affected callers (e.g. mktree --batch)
* Consider corrective measures

Steps 1-4 above are probably fine, and step 5 may have been where I
went off the rails.  My thinking there, wrong or right, was:

I think we should consider the log message independently from whether we
eventually return a value (whether QUICK or not). It seems like the log
message is often unnecessarily scary, because we either recover via
SECOND_READ, or we are in QUICK mode and the false negative is OK. So
the message is informative at best, and probably just noise in those
cases.

But it perhaps _is_ helpful when a non-QUICK lookup ends up returning
failure. We'll end up with some other error() message, but it may be
useful context to know that we _thought_ we had the object available and
then the rug was pulled out from under us. But we don't have a good way
of queuing up an error that is shown conditionally.

So I dunno. We could consider moving that message into trace/trace2,
making it more of a "debug" message. And then people digging into a
problem can turn on traces. But I have a feeling that is not very
helpful, since it is mostly a racy situation (so you can't just easily
replay your failure with tracing turned on).

* It makes sense that we don't want to reprepare most of the time
* ...but _if_ we know of the existence of some specific packfile in
this process and that packfile has since disappeared by the time we go
to open or read it, is that a special case?  Should it be?

So now we can consider the actual return value, aside from the logged
message. For non-QUICK requests, I think this case is uninteresting (we
already do a reprepare and follow-up read). For mktree, I think the core
of the problem is using QUICK when it should not.

I think the current behavior of QUICK is _correct_, in the sense that
false negatives are OK. But can we make it better? Possibly. To me the
argument for this patch's direction is something like:

  The point of QUICK was to avoid lots of reprepare effort when we are
  looking up objects that we might reasonably not have. This has
  historically been about things like fetch speculatively looking for
  stuff the other side mentioned. But there we are mostly concerned
  about objects we _never_ had, and avoiding tons of reprepare work that
  will almost certainly not help us. But in some races, we might learn
  that we _did_ have the object at one point (because we opened its idx,
  or a midx) but the lookup still failed (because the pack couldn't be
  accessed).

  We can cheaply notice this case by differentiating true idx misses
  from failure to access the pack contents. And these items _are_ worth
  a reprepare, because they were almost certainly caused by a repacking
  race (or a true repo corruption or object pruning, but that is rare
  enough not to worry about for optimization purposes).

  So even though QUICK is not _wrong_ to say "we do not have that
  object", it is a good tradeoff to spend a little bit of time calling
  reprepare in order to produce fewer false negative "no such object"
  responses (because tools like fetch then have a chance to optimize
  their own task more as a result).

Maybe that argument was somewhere in your original commit message. I
admit I got lost about half-way through. ;)

But I think the key thing is separating:

  - is the logging confusing or useful? What should we do about it?

  - is mktree racily broken because of QUICK? I think so.

  - even though QUICK is not wrong to skip the second read for this
    case, it might be a good tradeoff for it to detect and try harder
    here (i.e., the argument above).

Which sounds like three patches to me, each of which can be motivated
and argued on its own.

quoted

So I don't see QUICK itself here violating any contract (even if it
_could_ find the object in some cases with just a little more work, as
in the case that we were discussing for v1).

I'll drop this patch and instead send a small mktree change that stops
passing OBJECT_INFO_QUICK (keeping SKIP_FETCH_OBJECT), so mktree
recovers via the normal reprepare like every other non-QUICK reader.
That removes the packfile.c changes entirely, so both the
reload-under-QUICK hack and the .idx/.pack raciness you noted in
pack_index_is_missing() go away with them.

I am also happy with this direction. Then we can consider the other
questions separately (or not at all if nobody cares enough).

-Peff