Move datastore transition to attaching state to after summary retriev…

…ed (microsoft#20998)

the primary change is to move getting the attachment summary before
transitioning to attached for datastores. This also matches how the
container works, specifically, capture the summary, and then transition
to attaching. Since this code is all synchronous the risk of something
bad happening is small, or about the same as before.

Regardless of the order, having the summary retrieval decoupled from the
state transition leaves open the possibility for bugs. With this change
it is possible the dds state is modified after the summary is captured,
so it won't be in the summary or in ops, only in memory. Conversely, in
the existing code if a change happened after the attach state changed it
would get an op, but it would also be in the summary, so it gets
duplicated. Both of these are data corruption.

The better and much bigger fix would be to get the summary as part of
the transition, so as each ddses attachment summary is retrieved it then
transition to attaching. this would be a substantial refactoring, and
due to the code being synchronous the opportunity for these types of
issues is small, and would likely represent a framework issue.

Testing done as part of microsoft#20995

---------

Co-authored-by: Tony Murphy <anthonm@microsoft.com>

packages/runtime/container-runtime/src/channelCollection.ts

@@ -562,8 +562,8 @@ export class ChannelCollection implements IFluidDataStoreChannel, IDisposable {
 		 * If the container is detached, this data store will be part of the summary that makes the container attached.
 		 */
 		if (this.parentContext.attachState !== AttachState.Detached) {
-			localContext.setAttachState(AttachState.Attaching);
 			this.submitAttachChannelOp(localContext);
+			localContext.setAttachState(AttachState.Attaching);
 		}
 
 		this.contexts.bind(id);

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

@@ -31,9 +31,9 @@ const sharedPoints = [3, 4, 5];
 const ddsKey = "string";
 
 const testConfigs = generatePairwiseOptions({
-	containerAttachPoint: [ContainerCreated, DatastoreCreated, ...sharedPoints],
+	containerAttachPoint: [ContainerCreated, DatastoreCreated, DdsCreated, ...sharedPoints],
 	containerSaveAfterAttach: [true, false],
-	datastoreAttachPoint: [DatastoreCreated, ...sharedPoints],
+	datastoreAttachPoint: [DatastoreCreated, DdsCreated, ...sharedPoints],
 	datastoreSaveAfterAttach: [true, false],
 	ddsAttachPoint: [DdsCreated, ...sharedPoints],
 	ddsSaveAfterAttach: [true, false],
@@ -132,10 +132,15 @@ describeCompat("Validate Attach lifecycle", "FullCompat", (getTestObjectProvider
 						);
 					}
 				};
-				if (testConfig.ddsAttachPoint === 2) {
-					// point 2 - at dds create
+				if (testConfig.ddsAttachPoint === DdsCreated) {
 					await attachDds();
 				}
+				if (testConfig.datastoreAttachPoint === DdsCreated) {
+					await attachDatastore();
+				}
+				if (testConfig.containerAttachPoint === DdsCreated) {
+					await attachContainer();
+				}
 
 				// all objects, container, datastore, and dds are created, at least in memory at this point
 				// so now we can attach whatever is not in the presence of all the others