Fix ETag retrieval for orchestrations with multiple executions

[ConnorMcMahon]

It is possible for the history table to get into a state where the
latest execution history is first, followed by the previous execution
history, with the last entry for an instance id being the sentinal row.
The previous history parsing logic stopped processing as soon as it saw
a new execution id, meaning it failed to grab the ETag from the sentinal
row.

This, in combination with changes made when the ETag were not present,
caused UpdateState calls to fail, meaning queue messages for Activities
were being scheduled without recording that we did schedule them. This
leads to duplicate Activity executions until we retrieve the history
with an execution id (i.e. after an activity function successfully
executes).

This fix makes our ETag retrieval logic more robust.

[ConnorMcMahon]

Debatably this should be an error and an exception should be thrown here. However, I am a bit hesitant throwing an exception, as I'm not sure how it is handled here.

[cgillum]

I see what you're trying to do but this last refactor seems unnecessarily complicated. Here is my suggestion, which I think is simpler and does not strictly require the sentinel to be the last row:

IList<HistoryEvent> historyEvents;
string executionId;
DynamicTableEntity sentinel = null;
if (tableEntities.Count > 0)
{
    // The most recent generation will always be in the first history event.
    executionId = tableEntities[0].Properties["ExecutionId"].StringValue;

    // Convert the table entities into history events.
    var events = new List<HistoryEvent>(tableEntities.Count);

    foreach (DynamicTableEntity entity in tableEntities)
    {
        if (entity.Properties["ExecutionId"].StringValue != executionId)
        {
            // The remaining entities are from a previous generation and can be discarded.
            break;
        }

        if (entity.RowKey == SentinelRowKey)
        {
            sentinel = entity;
            continue;
        }

        // Some entity properties may be stored in blob storage.
        await this.DecompressLargeEntityProperties(entity);

        events.Add((HistoryEvent)this.tableEntityConverter.ConvertFromTableEntity(entity, GetTypeForTableEntity));
    }

    historyEvents = events;
}
else
{
    historyEvents = EmptyHistoryEventList;
    executionId = expectedExecutionId;
}

// Read the checkpoint completion time from the sentinel row, which should always be the last row.
// The only time a sentinel won't exist is if no instance of this ID has ever existed.
// The IsCheckpointCompleteProperty was newly added _after_ v1.6.4.
DateTime checkpointCompletionTime = DateTime.MinValue;
if (sentinel == null)
{
    sentinel = tableEntities.LastOrDefault(e => e.RowKey == SentinelRowKey);
}

string eTagValue = sentinel?.ETag;
if (sentinel != null &&
    sentinel.Properties.TryGetValue(CheckpointCompletedTimestampProperty, out EntityProperty timestampProperty))
{
    checkpointCompletionTime = timestampProperty.DateTime ?? DateTime.MinValue;
}

[sebastianburckhardt]

Looks good to me. Found one typo in comment, and have a slight suggestion on wording.

[sebastianburckhardt]

sentinal => sentinel

[sebastianburckhardt]

... or it was removed via a history purge management operation?

[cgillum]

- LGTM!

[cgillum]

Though it looks like the test run timed out. Let's make sure the CI is healthy before merging.

[ConnorMcMahon]

I rebased onto master just to retrigger the build, and it appears to have worked this time. The timeout last time was in a Service Bus test, which I didn't touch here, so I am going to merge now.