Race condition when inserting records about ranked-model

[brendon]

Fair enough :) I think the best way forward would be to look for a guaranteed solution rather than one that reduces the risk.

At least the bug is fixed now :D

[brendon]

Hi @fschwahn, that error rings a bell. I thought we'd fixed it or at least guarded against it. I assume you don't have a default value on that column as that would be prevent on boot.

Would you be interested in looking at a solution to this one? I suspect it'll involve locking of some kind :)

[fschwahn]

I've looked a bit into it, and I found the problem:

ranked-model/lib/ranked-model/ranker.rb

Lines 295 to 303 in c8ebb37

	def current_last
	@current_last ||= begin
	if (ordered_instance = finder.
	reverse.
	first)
	RankedModel::Ranker::Mapper.new ranker, ordered_instance
	end
	end
	end

Calling reverse on finder loads the loads the ActiveRecord::Relation into memory (ie. finder.loaded? returns true). Every subsequent call to finder.first does no DB lookup anymore, but takes the result from the loaded relation in memory. In concurrent settings this loaded relation might be outdated (in my example returns nil instead of a record).

Loading the entire relation into memory seems never a good idea here, as only one record is of interest, so an immediate fix would be to use last so not the entire relation is loaded:

if (ordered_instance = finder.last)

I know too little about the code to tell if this has other side-effects, but it seems like a safe change.

That's a good idea in any case. However, in case current_first already returned something, it is memoized in an ivar, and might also be outdated. So it might make sense to call reset_cache at the top of rearrange_ranks. However, it is much less obvious to me if this has other side-effects.

[brendon]

That makes a lot of sense :) Even with .last I suspect there's still a chance for stale data unless we wrap all of this in a locking transaction?

The decision for .reverse was a bit bizarre:

26d35cf

But it was a simplification of what came before it, though prior to that the code was genuinely trying to reverse the sort order of the query. That code it a lot simpler these days with modern Rails :)

I'll close this for now, but let me know if you have any more thoughts on how to tighten this up.

[fschwahn]

I suspect there's still a chance for stale data unless we wrap all of this in a locking transaction?

Yes, I think so. However I don't know much about pessimistic locking, and I'd be afraid of introducing deadlocks 😐

I'll close this for now, but let me know if you have any more thoughts on how to tighten this up.

One more idea I had was introducing an optional jitter (applied in rank_at_average) to reduce the likelihood that 2 items are given the exact same row_order value (which is what ultimately leads to this issue here). However, that might not be desirable in situations where lots of re-ordering happens and the entire available space must be used.