Possible issue in SimpleQueueCacheCursor.RecordDeliveryFailure

[KevinCathcart]

I'm looking at the code for SimpleQueueCacheCursor.RecordDeliveryFailure(). That code is called by the PersistentStreamPullingAgent when the current value retrieved from the cursor could not be delivered. The function looks like this:

orleans/src/OrleansProviders/Streams/Common/SimpleCache/SimpleQueueCacheCursor.cs

Lines 111 to 120 in 7090d7d

	/// <summary>
	/// Record that delivery of the current event has failed
	/// </summary>
	public void RecordDeliveryFailure()
	{
	if (IsSet && current != null)
	{
	Element.Value.DeliveryFailure = true;
	}
	}

The confusing thing is that Element is documented as pointing to the next element after current. So it would seem like the DeliveryFailure flag is being set on the element after the one that failed. Which might not be much of a problem as long as such an element always exists, and the code that consumes consumes this flag looks ahead (but that would certainly warrant a comment). However there is not always a next item, and it looks like the code that consumes this (SimpleQueueCacheItem.DrainBucket) does not look ahead.

So am I missing something, or is this buggy?

If this is buggy, fixing this directly seems tricky, because the value in the cursor's current field may no longer even be in the cache, since it could be in the previous cache bucket relative to Element, and therefore would be eligible for cleanup.

[KevinCathcart]

Another consideration is that the pattern where the messages that fail simply never get passed to IQueueAdapterReceiver.MessagesDeliveredAsync() works fine for Azure queues because they simply timeout, but is less than optimal for some other providers, which want or need to explicitly release the message to be re-queued (e.g. if that only happens automatically if the connection is lost).

Right now the only way to do that without forking the SimpleQueueCache, is to maintain a sequence number to message lookup table, and use a custom IStreamFailureHandler, which is still suboptimal for exactly the same reasons that Jason mentioned back when implementing RecordDeliveryFailure() in the first place.

See my comment on a Stream provider's pull request for an example of a provider where explicitly releasing a message would be desirable.

[KevinCathcart]

Hmm, my offhand comment about the cursor's current property potentially already being removed from the cache, is a bigger problem than I thought. I'm pretty sure that is the entire issue being reported in #6281.

I'm also pretty sure this can cause at-least-once delivery to not occur if the last message in bucket got handed to MessagesDeliveredAsync, while it was actually undergoing delivery attempts that would ultimately end up failing.

I have an idea for fixing both issues with a relatively small code change, so I might be able to post a possible PR sometime tommorrow if it looks like that actually works. Unfortunately it won't do much to help with providers that want to actively request requeueing, which probably needs a larger design change.