Fix join source remap feed race

[lyang24]

I confirm that this contribution is made under the terms of the license found in the root directory of this repository's source tree and that I have the authority necessary to make this contribution on behalf of its copyright owner.

A feed operation checks that its source bucket exists and schedules the operation while holding the bucket DB entry lock. Previously, join remapped queued operations before acquiring this lock to remove the source bucket.

This left a window where a feed operation could be scheduled after the queue remap but before the source bucket was removed. The operation would then be stranded in the queue for a bucket that no longer existed in the bucket DB.

The join path now acquires the source bucket DB entry lock before remapping queued operations and holds it until the source entry has been removed. This ensures that a concurrent feed operation either:

• is scheduled before the remap and gets remapped to the target bucket, or
• observes the removed source bucket and is remapped or rejected.

This also makes the join path consistent with the existing split lock ordering.

[vekterli]

The lock order change in splitjoinhandler.cpp looks appropriate, and I don't think the original order was intentional (especially since the split case was the other way around).

However, I think the added test can be removed, since it does not actually test any observable behaviors of the component in question (but does add a test hook to the production code and depends on an undocumented, unstable internal debug API). I personally consider locking order invariants an internal detail of the implementation that's more appropriately checked by TSan/fuzzing/stress testing rather than a unit test, so just having a proper documented rationale in the code should suffice.

Also, a question:

The operation would then be stranded in the queue for a bucket that no longer existed in the bucket DB.

What is the actual observed effects of an operation getting "stranded" in the queue? My empirical expectation of such an edge case is that it would most likely cause the search core and the content layer bucket states to get out of sync, but not cause any noticeable stalls as per se. The sync issue could remain until the next process restart (at which point all bucket state is reloaded from the search core), but it's also likely that the sending distributor process is usually able to observe the inconsistency and silently trigger a repair almost immediately. That would explain why this edge case has been present for such a long time.

I'll allow myself to quote a poignant observation from the 2007 Google paper "Paxos Made Live - An Engineering Perspective" 🙂

In closing we point out a challenge that we faced in testing our system for which we have no systematic
solution. By their very nature, fault-tolerant systems try to mask problems. Thus they can mask bugs or
configuration problems while insidiously lowering their own fault-tolerance.

[lyang24]

The lock order change in splitjoinhandler.cpp looks appropriate, and I don't think the original order was intentional (especially since the split case was the other way around).

However, I think the added test can be removed, since it does not actually test any observable behaviors of the component in question (but does add a test hook to the production code and depends on an undocumented, unstable internal debug API). I personally consider locking order invariants an internal detail of the implementation that's more appropriately checked by TSan/fuzzing/stress testing rather than a unit test, so just having a proper documented rationale in the code should suffice.

Also, a question:

The operation would then be stranded in the queue for a bucket that no longer existed in the bucket DB.

What is the actual observed effects of an operation getting "stranded" in the queue? My empirical expectation of such an edge case is that it would most likely cause the search core and the content layer bucket states to get out of sync, but not cause any noticeable stalls as per se. The sync issue could remain until the next process restart (at which point all bucket state is reloaded from the search core), but it's also likely that the sending distributor process is usually able to observe the inconsistency and silently trigger a repair almost immediately. That would explain why this edge case has been present for such a long time.

I'll allow myself to quote a poignant observation from the 2007 Google paper "Paxos Made Live - An Engineering Perspective" 🙂

In closing we point out a challenge that we faced in testing our system for which we have no systematic
solution. By their very nature, fault-tolerant systems try to mask problems. Thus they can mask bugs or
configuration problems while insidiously lowering their own fault-tolerance.

you are right the stranded is overstated. As your noted the impact is over stated, the distributor may detect and repair this, and a restart would rebuild the bucket DB from Proton.

Very true—“fault-tolerant systems try to mask problems” captures this case well. Deterministic simulation and fault injection may at least help expose these otherwise silent failures.