Fetch protocol locking order is unsafe

[lucamilanesio]

Version

v7.4.0.202509020913-r

Operating System

MacOS, Linux

Bug description

When performing a forced fetch operation using FetchProcess.execute(), the fetch is supposed to lock the local ref until the fetch completes.

Locking the local ref is paramount because it would risk losing or overwriting any ref updates that occur locally whilst the fetch operation is in progress.

For detecting local modifications since the start of the fetch, the FetchProcess.want() reads the local ref at the time of the fetch and stores it into the expected object ID of the ref-update.
The ref-update upon completion checks whether the local ref has changed and, if so, returns a LOCK_FAILURE error, aborting the operation.

Actual behavior

1. Start a FetchProcess.execute() of a remote ref refs/heads/remotefoo (value=2) into a local ref refs/heads/localfoo (value=1)
2. Update the local ref refs/heads/localfooo (value=3)
3. Wait for the FetchProcess.execute() to finish

What I see is that depending on when step 2. happens in the middle of the FetchProcess.execute(), the FetchProcess.execute() may succeed and point to value = 2.

Expected behavior

After step 3, the FetchProcess.execute() should fail with a LOCK_FAILURE and the refs/heads/localfoo should have the value=3, which is the update at step 2 that won the race.

The modification happens during the execution of the command and shouldn't silently overwrite the destination ref depending on the concurrency and timing of the local operation.

Relevant log output

Other information

I believe the problem is in the sequence of the logical locking of the refs/heads/localfoo.

The fetch of the refs/heads/localfoo happens in the want(Ref src, RefSpec spec) function, when the remote ref and its value have already been fetched; however, things may have changed because nothing was locked before then.

If the local ref-update occurs after the execution of want(Ref src, RefSpec spec), it is detected, but any prior modification would go unnoticed and therefore could be lost or overwritten.

[mfick-nvidia]

Hmm, did you make a typo in your expected behavior? You said you would expect it to have value=1, I would never expect that since I would expect that either the fetch or the local update would succeeds (but not both), in which case I would expect the value to be either 2 (fetch succeeded), or 3 (local update succeeded).

[lucamilanesio]

Hmm, did you make a typo in your expected behavior?

Yes, I did, well spotted, thanks! 👍

You said you would expect it to have value=1, I would never expect that since I would expect that either the fetch or the local update would succeeds (but not both), in which case I would expect the value to be either 2 (fetch succeeded), or 3 (local update succeeded).

Yep, correct. I would expect the update to win the race and the fetch to detect that the local ref changed since the fetch started, and failing its operation with a LOCK_FAILURE.