Fix race condition in recording results

[ChrisBr]

I believe there is a race condition between acknowledge and record.

When we call acknowledge it will immediately remove the test from the running set. We only record failure in the next step in record. However, we check the running set in exhausted? in the summary to decide when the queue is empty. There is a possibility of removing a test from the running set and the summary exiting before we've actually recorded the failure. acknowledge and record should be a single operation to avoid this.

ci-queue/ruby/lib/minitest/queue.rb

Lines 254 to 255 in dd85b49

	elsif queue.acknowledge(example)
	reporter.record(result)

ci-queue/ruby/lib/ci/queue/redis/base.rb

Lines 104 to 106 in dd85b49

	def exhausted?
	queue_initialized? && size == 0
	end

ci-queue/ruby/lib/ci/queue/redis/base.rb

Lines 121 to 126 in dd85b49

	def size
	redis.multi do |transaction|
	transaction.llen(key('queue'))
	transaction.zcard(key('running'))
	end.inject(:+)
	end

ci-queue/ruby/lib/ci/queue/redis/supervisor.rb

Line 29 in dd85b49

until exhausted? || time_left <= 0 || max_test_failed? || time_left_with_no_workers <= 0

ci-queue/redis/acknowledge.lua

Lines 7 to 8 in dd85b49

	redis.call('zrem', zset_key, test)
	redis.call('hdel', owners_key, test) -- Doesn't matter if it was reclaimed by another workers

[ChrisBr]

TODO: Find new location for ErrorReport and FailureFormatter. This may break public API?

[ChrisBr]

@byroot any opinion on this? RspecQueue makes this quite a difficult refactor... The other option is to move acknowledge down into the reporter and call it as part of report_success / report_error which may be simpler.

[byroot]

any opinion on this?

I haven't spent too much time looking at the implemementation, but yes, the error has to be recorded in the same transaction as the test is acknowledged for it to be fully safe, which is what I do in megatest: https://github.com/byroot/megatest/blob/16fcf099b78fd9d9f8b0b5e5132beff30f5285f2/lib/megatest/redis_queue.rb#L300-L320 (should be a multi instead of a pipeline though).

But the challenge as you noticed is to get all that data at once, which is challenging when there isn't an extension API designed to give you that.

For the rspec part, IIRC it was marked as deprecated ages ago, you could just remove it if it's really in the way.