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:

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).

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.

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

-Peff