Refresh immediately after summary ack #18968

tyler-cai-microsoft · 2023-12-21T22:48:02Z

Background

Attempting AB#3400 to add validation to the summarizer before summaries occurred to verify that the summary the running summarizer is tracking is the same as the summarizer node system, we discovered that the validation was failing. The validation caught locally that a single summarizer would start a second summary before processing the ack of the first summary it created.

Solution

This PR attempts to address that issue. Instead of waiting for the summary lock to be released, the ack is immediately refreshed. This may result in multiple refreshes. The ContainerRuntime's refresh logic is resilient and should be able to handle refreshing multiple of the same acks.

Further Work

The RunningSummarizer really only expects its own acks. Any foreign acks should close/restart the summarizer. The code in the RunningSummarizer should reflect that reality.

packages/runtime/container-runtime/src/summary/runningSummarizer.ts

packages/runtime/container-runtime/src/summary/summaryGenerator.ts

packages/runtime/container-runtime/src/summary/runningSummarizer.ts

agarwal-navin · 2023-12-22T17:50:46Z

packages/test/test-end-to-end-tests/src/test/summarizeRefreshLatestSummary.spec.ts

+				await waitForContainerConnection(container);
+				await waitForContainerConnection(container2);
+
+				// The first summary needs to be generated otherwise the summarizer will only generate one summary.


I don't know, I was running the test without generating the first summary and it would not work. I'll have to dig into this to answer.

Specifically this doesn't work because the retry logic based on the call to trySummarize requires the summarizerLock to be defined. Because it's undefined the heuristics runner doesn't run in the afterSummaryAction. So essentially the first summary needs to occur to enable the summary lock to be locked, and then a refresh needs to happen in order to enable the lock. It's a race condition.

agarwal-navin · 2023-12-22T17:51:26Z

packages/test/test-end-to-end-tests/src/test/summarizeRefreshLatestSummary.spec.ts

+				dataObject._root.set(`first`, "op");
+				await provider.ensureSynchronized();
+
+				const array: string[] = [];


What is this array doing? And why does container3 load from the first ack?

To be honest, I was curious as to what the behavior of the container would be, whether or not the 2nd op would be summarized or not. Based on the tests, with the local server, the 2nd op would be summarized in 1st the summary. With the tinylicious server, I think the 2nd op would not be summarized until the 2nd summary. It's interesting behavior that I wasn't sure if it was important to note. The summaries generated would be different, but the document should recover/become eventually consistent. This is a rare edge case.

Another note, the array should contain 2 acks, and I should check for that at the end of the test. Another note, if we update the summary heuristics runner in any fashion, this test will likely start failing.

agarwal-navin · 2023-12-22T17:52:40Z

packages/test/test-end-to-end-tests/src/test/summarizeRefreshLatestSummary.spec.ts

+					}
+				});
+
+				// Sending ops from two different clients/containers causes the heuristics summarizer to handle ops twice, which causes the summarizer to summarize before acking its first summary.


There is no validation that the second summarize is triggered before the first one is acked. You should add that validation somehow otherwise, it makes the test undetereministic. It may not be testing what is claimed here as everything is async.

I didn't know exactly how to add in that validation without just starting to cast to the actual internal runtime implementation. What needs to happen is that the running summarizer listens to every op and figures out if it needs to run the summary heuristics runner, so I set the max ops threshold very low (1 op). Then during the summary itself, there need to be unprocessed ops that sit in the inbound queue that again trigger the heuristics runner from the afterSummaryAction (or a timer, but ops is simpler). I have two containers send different ops, because when I try to do it all in one container, maybe all the ops get batched into one op (or are processed together) which doesn't trigger the summary. I'm not exactly sure if what's the case, but when I did try it with 1 container I could not effect the same result. The second op from the second container should trigger the 2nd summary as it sits in the queue unprocessed as the run summary is triggered synchronously. I'm not in love with my repro, but if there's a better way to implement it and write it so it's more testable, I'm all ears.

agarwal-navin · 2023-12-22T17:52:51Z

packages/test/test-end-to-end-tests/src/test/summarizeRefreshLatestSummary.spec.ts

+				});
+
+				const dataObject3 = (await container3.getEntryPoint()) as ITestDataObject;
+				assert(dataObject3._root.get(`a`) === "op1");


Why is this validation needed?

This helps test eventual consistency. There's actually some interesting behavior here that may be worth investigating as this is an edge case.

packages/test/test-end-to-end-tests/src/test/summarizeRefreshLatestSummary.spec.ts

packages/runtime/container-runtime/src/summary/runningSummarizer.ts

msfluid-bot · 2024-01-04T01:37:43Z

Baseline CI build failed, cannot generate bundle analysis at this time

Baseline commit: 22904a9

Generated by 🚫 dangerJS against f469edd

packages/runtime/container-runtime/src/summary/summaryGenerator.ts

agarwal-navin · 2024-01-24T00:00:09Z

packages/test/test-end-to-end-tests/src/test/summarizeRefreshLatestSummary.spec.ts

+			await waitForContainerConnection(container);
+			await waitForContainerConnection(container2);
+
+			// The first summary needs to be generated otherwise the summarizer will only generate one summary.


I did not understand what this comment means.

Added additional clarifying comments. This is to put the summarizer client in the "right" state for conditions of two summaries occurring simultaneously one after another.

agarwal-navin · 2024-01-24T00:03:31Z

packages/test/test-end-to-end-tests/src/test/summarizeRefreshLatestSummary.spec.ts

+			// The first summary needs to be generated otherwise the summarizer will only generate one summary.
+			dataObject._root.set(`first`, "op");
+			await provider.ensureSynchronized();
+			mockLogger.assertMatch([


Just waiting for the op to be processed is not enough. It doesn't guarantee that the summary would have been generated and can result in this test being flaky.

Is it possible to reliably test this in an end-to-end test? I am not sure what you could wait on to validate that things happen as expected.
I would do something like just call the summarize function on summary generator and validate that refresh happens. Maybe in a unit test.

I limited this to just local server, do you think it'll still be flakey here?

agarwal-navin · 2024-01-24T00:03:51Z

packages/test/test-end-to-end-tests/src/test/summarizeRefreshLatestSummary.spec.ts

+			const twoSummariesDeferred = new Deferred<void>();
+			const summaryVersions: string[] = [];
+			let summariesSeen = 0;
+			container.on("op", (op) => {


There is a lot going on here. Can you add a comment explaining what's going on.

In order to locally cause two summaries to be generated in a row, I have to do this weird dance, I'm wondering if this is such a freak scenario that it's not happening at any decent level in production.

packages/runtime/container-runtime/src/summary/summaryGenerator.ts

[AB#3400](https://dev.azure.com/fluidframework/internal/_workitems/edit/3400) Updated from previous PR: #18754 as it was getting stale. ## Description After ensuring that we've refreshed our latest summary state: #18968, this PR adds validation that before we start a summary, that all the summary sequence numbers are as expected from the `RunningSummarizer` down to the `SummarizerNode` system itself. This is to prevent any sort of data corruption/data loss from accidentally occurring. This check used to run in the two-commit summary system, which would prevent us from submitting a bad summary. Now that we've moved to the one-commit summary system, this should keep us safe from potentially negatively impacting documents. ## What's in the PR This PR adds validation that the summary system from the RunningSummarizer down to the SummarizerNode system that the starting sequence numbers are correct. The code is behind a feature flag. There is telemetry that's added so that we can see before we even turn on the feature what the impact to the entire system will look like. --------- Co-authored-by: Mark Fields <markfields@users.noreply.github.com>

[AB#6490](https://dev.azure.com/fluidframework/internal/_workitems/edit/6490) ## Background Attempting [AB#3400](https://dev.azure.com/fluidframework/internal/_workitems/edit/3400) to add validation to the summarizer before summaries occurred to verify that the summary the running summarizer is tracking is the same as the summarizer node system, we discovered that the validation was failing. The validation caught locally that a single summarizer would start a second summary before processing the ack of the first summary it created. ## Solution This PR attempts to address that issue. Instead of waiting for the summary lock to be released, the ack is immediately refreshed. This may result in multiple refreshes. The `ContainerRuntime`'s refresh logic is resilient and should be able to handle refreshing multiple of the same acks. ## Further Work The `RunningSummarizer` really only expects its own acks. Any foreign acks should close/restart the summarizer. The code in the `RunningSummarizer` should reflect that reality.

[AB#3400](https://dev.azure.com/fluidframework/internal/_workitems/edit/3400) Updated from previous PR: microsoft#18754 as it was getting stale. ## Description After ensuring that we've refreshed our latest summary state: microsoft#18968, this PR adds validation that before we start a summary, that all the summary sequence numbers are as expected from the `RunningSummarizer` down to the `SummarizerNode` system itself. This is to prevent any sort of data corruption/data loss from accidentally occurring. This check used to run in the two-commit summary system, which would prevent us from submitting a bad summary. Now that we've moved to the one-commit summary system, this should keep us safe from potentially negatively impacting documents. ## What's in the PR This PR adds validation that the summary system from the RunningSummarizer down to the SummarizerNode system that the starting sequence numbers are correct. The code is behind a feature flag. There is telemetry that's added so that we can see before we even turn on the feature what the impact to the entire system will look like. --------- Co-authored-by: Mark Fields <markfields@users.noreply.github.com>

Refresh immediately after summary ack

f4d5644

tyler-cai-microsoft requested review from a team as code owners December 21, 2023 22:48

github-actions bot added area: runtime Runtime related issues area: tests Tests to add, test infrastructure improvements, etc base: main PRs targeted against main branch labels Dec 21, 2023

Add missing await and update test, fix flag

eeb8073

tyler-cai-microsoft requested a review from agarwal-navin December 22, 2023 00:02

tyler-cai-microsoft commented Dec 22, 2023

View reviewed changes

packages/runtime/container-runtime/src/summary/runningSummarizer.ts Outdated Show resolved Hide resolved

tyler-cai-microsoft commented Dec 22, 2023

View reviewed changes

packages/runtime/container-runtime/src/summary/summaryGenerator.ts Outdated Show resolved Hide resolved

agarwal-navin reviewed Dec 22, 2023

View reviewed changes

packages/runtime/container-runtime/src/summary/runningSummarizer.ts Outdated Show resolved Hide resolved

agarwal-navin reviewed Dec 22, 2023

View reviewed changes

packages/runtime/container-runtime/src/summary/runningSummarizer.ts Outdated Show resolved Hide resolved

agarwal-navin reviewed Dec 22, 2023

View reviewed changes

packages/runtime/container-runtime/src/summary/runningSummarizer.ts Show resolved Hide resolved

agarwal-navin reviewed Dec 22, 2023

View reviewed changes

packages/test/test-end-to-end-tests/src/test/summarizeRefreshLatestSummary.spec.ts Outdated Show resolved Hide resolved

rajatch-ff reviewed Dec 25, 2023

View reviewed changes

packages/runtime/container-runtime/src/summary/runningSummarizer.ts Show resolved Hide resolved

rajatch-ff reviewed Dec 25, 2023

View reviewed changes

rajatch-ff requested a review from a team December 29, 2023 21:21

tyler-cai-microsoft and others added 7 commits January 2, 2024 10:16

Merge branch 'main' into refresh-immediately-after-summary-ack

e864790

Fix tests add mock logger events

c81ca05

Merge branch 'main' into refresh-immediately-after-summary-ack

1f88393

Merge branch 'main' into refresh-immediately-after-summary-ack

2083d25

Wait for two summaries

85eabd6

Merge branch 'main' into refresh-immediately-after-summary-ack

06e2371

Skip non local driver tests

cba37d4

Slight refactoring

330005b

agarwal-navin reviewed Jan 23, 2024

View reviewed changes

packages/runtime/container-runtime/src/summary/summaryGenerator.ts Show resolved Hide resolved

agarwal-navin reviewed Jan 24, 2024

View reviewed changes

tyler-cai-microsoft added 6 commits January 30, 2024 15:59

Merge branch 'main' into refresh-immediately-after-summary-ack

66c795c

Merge branch 'main' into refresh-immediately-after-summary-ack

725aa23

Merge branch 'main' into refresh-immediately-after-summary-ack

eb431c1

Make test easier to understand

1ae9926

Missing comment changes

73f5247

Add comment

f469edd

NicholasCouri reviewed Feb 1, 2024

View reviewed changes

packages/runtime/container-runtime/src/summary/summaryGenerator.ts Show resolved Hide resolved

NicholasCouri approved these changes Feb 1, 2024

View reviewed changes

tyler-cai-microsoft merged commit a940e0b into microsoft:main Feb 1, 2024
30 checks passed

tyler-cai-microsoft mentioned this pull request Feb 5, 2024

Redo summarizer correctness validation #19488

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refresh immediately after summary ack #18968

Refresh immediately after summary ack #18968

tyler-cai-microsoft commented Dec 21, 2023 •

edited

agarwal-navin Dec 22, 2023

tyler-cai-microsoft Jan 2, 2024

tyler-cai-microsoft Jan 4, 2024

agarwal-navin Dec 22, 2023

tyler-cai-microsoft Jan 2, 2024

tyler-cai-microsoft Jan 2, 2024

agarwal-navin Dec 22, 2023

tyler-cai-microsoft Jan 2, 2024

agarwal-navin Dec 22, 2023

tyler-cai-microsoft Jan 2, 2024

tyler-cai-microsoft Jan 2, 2024

msfluid-bot commented Jan 4, 2024 •

edited

agarwal-navin Jan 24, 2024

tyler-cai-microsoft Jan 31, 2024

agarwal-navin Jan 24, 2024

tyler-cai-microsoft Jan 31, 2024

agarwal-navin Jan 24, 2024

tyler-cai-microsoft Jan 31, 2024 •

edited

Refresh immediately after summary ack #18968

Refresh immediately after summary ack #18968

Conversation

tyler-cai-microsoft commented Dec 21, 2023 • edited

Background

Solution

Further Work

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msfluid-bot commented Jan 4, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tyler-cai-microsoft Jan 31, 2024 • edited

Choose a reason for hiding this comment

tyler-cai-microsoft commented Dec 21, 2023 •

edited

msfluid-bot commented Jan 4, 2024 •

edited

tyler-cai-microsoft Jan 31, 2024 •

edited