Add UT coverage for lastSummary successfully happening when main client disconnects due to nack (10K ops)

[vladsud]

Tracking work mentioned in #7247 (comment)

Navin: Is it possible to re-create this scenario in tests? Basically, making sure that docs can get out of broken state when it goes through the cycle of reconnects

Vlad: I'll look into it.

This is RE this comment:

this.delayBeforeCreatingSummarizer().then(async (isDelayed: boolean) => {
// Re-validate that it need to be running. Due to asynchrony, it may be not the case anymore
// but only if creation was delayed. If it was not, then we want to ensure we always create
// a summarizer to kick off lastSummary. Without that, we would not be able to summarize and get
// document out of broken state if it has too many ops and ordering service keeps nacking main
// container (and thus it goes into cycle of reconnects)

[vladsud]

Note: that's special case of #7292

[pleath]

@vladsud, @agarwal-navin, do we know that this works correctly today? In my attempts to test a case where the SummaryManager disconnects while we're in the delay before creating the summarizer, we don't generate the last summary before exiting. The last summary gets generated correctly if we're stopping a running summarizer but not if we're still in the Starting state.

[agarwal-navin]

It may not work as expected, hence the ask to have tests for this scenario :-). Can you share the test to give an idea of the exact scenario?

[vladsud]

@pleath, you are correct here. (all form my memory, without looking at the code :)) Basically if we have less than 4K unsummarized ops when we start the process, then we will wait some time, and we will recheck if given client is still a summarizer after wait, and exit if it's not, even if we have by that time over 4K ops. I'm not sure if it's interesting case.

The more important case is when we start with over 4K ops. Then there is no wait. We skip the test if current client is a summarizing client. Thus we always create summarizer and it will do last summary immediately (as it is instructed to exit right away). That's the main thing we should test (that it stays same way).

[pleath]

Agreed about the relative importance of the two cases. So you want to cover the specific case where we create the summarizer without a delay, but the client still manages to disconnect while we're creating the summarizer? So, this early return in SummaryManager::startSummarization:

        const summarizer = await this.requestSummarizerFn();

        // Re-validate that it need to be running. Due to asynchrony, it may be not the case anymore
        const shouldSummarizeState = this.getShouldSummarizeState();
        if (shouldSummarizeState.shouldSummarize === false) {
            summarizer.stop(shouldSummarizeState.stopReason);
            return;
        }

[vladsud]

I think the main thing to validate (in presence of too many ops) are:

• we do not exit early due to this check
• we do not exit anywhere else on similar checks, and actually produce last summary that gets client out of the misery.

I.e. the moment this client connects successfully to socket, if we queue disconnect (as a mini-task, or setTimeout(0)), then this single client in a document does successfully produce a summary.

[pleath]

Thanks. It seems like the relevant code is in SummaryManager (the delay logic on summaizer creation) and RunningSummarizer (running last summary on summarizer exit). So I think this could be accomplished by using a real RunningSummarizer in the SummaryManager UT, which doesn't use a real Summarizer.