row_cache: update _prev_snapshot_pos even if apply_to_incomplete() is preempted

[michoecho]

Commit e81fc1f accidentally broke the control flow of row_cache::do_update().

Before that commit, the body of the loop was wrapped in a lambda. Thus, to break out of the loop, return was used.

The bad commit removed the lambda, but didn't update the return accordingly. Thus, since the commit, the statement doesn't just break out of the loop as intended, but also skips the code after the loop, which updates _prev_snapshot_pos to reflect the work done by the loop.

As a result, whenever apply_to_incomplete() (the updater) is preempted, do_update() fails to update _prev_snapshot_pos. It remains in a stale state, until do_update() runs again and either finishes or is preempted outside of updater.

If we read a partition processed by do_update() but not covered by _prev_snapshot_pos, we will read stale data (from the previous snapshot), which will be remembered in the cache as the current data.

This results in outdated data being returned by the replica. (And perhaps in something worse if range tombstones are involved. I didn't investigate this possibility in depth).

Note: for queries with CL>1, occurences of this bug are likely to be hidden by reconciliation, because the reconciled query will only see stale data if the queried partition is affected by the bug on on all queried replicas at the time of the query.

Fixes #16759

[michoecho]

Affects all versions since 3.3.

[scylladb-promoter]

🟢 CI State: SUCCESS

✅ - Build
✅ - Container Test
✅ - dtest
✅ - Unit Tests

Build Details:

• Duration: 1 hr 50 min
• Builder: spider1.cloudius-systems.com

[tgrabiec]

Good catch. We should add some kind of unit test for this scenario.

[avikivity]

Very good. Please follow up with a unit test or an improvement to the randomized stress tests.

In addition we should have integration tests with RF=1 to prevent read repair from covering up such bugs. @roydahan let's add something to S-C-T scenarios.

[bhalevy]

Good catch. We should add some kind of unit test for this scenario.

Agreed (on both :)).
@michoecho, how did you reproduce the issue?

[michoecho]

@michoecho, how did you reproduce the issue?

@bhalevy Reproduce? From the issue report and from the source code of janusgraph, it appeared that the problem is happening even with just basic inserts and selects. And given the reported circumstances, both mine and @tgrabiec's intuition was that this felt like it involved a race between cache population and cache update. So I wrote a workload which attempts to cause such a race, by spamming a scenario of the form: insert, wait for several seconds (the duration of flush) and select. And this turned out to be enough.

I had a list of other ideas to try, but the first one worked.

[mykaul]

I think it was backported everywhere and we can remove the label, no?

[mykaul]

I think it was backported everywhere and we can remove the label, no?

@michoecho - can we remove the label? Anything else missing?

[michoecho]

I think it was backported everywhere and we can remove the label, no?

@michoecho - can we remove the label? Anything else missing?

We can remove the label.