Improvements in pending state transitions related to presence of clientId

[vladsud]

I might be wrong with the direction of this PR - it's based on my understanding / learning of how code works, based on tracing code and running through tests. If I'm wrong, consider that my learning exercise :)

I started with making one line change in serializedStateManager.ts, in order to bring more consistency (as part of #20767) to states we present to the user, including through reloads.

But I run into a bunch of problems that do look like bugs, or at lease very hard to follow implicit dependencies that are not documented anywhere.

1. Dropping clientId when there is no pending state makes sense (as being correct), but nothing tells that is has to happen, and I do not see any explicit dependency in runtime that it relies on it.
2. If container runtime can't rely on it, then it has to preserve this.savedOps always, as these will be counted as local ops and things will explode if pendingStateManger can't find them.
3. The check in ContainerRuntime.getPendingLocalState() looks fishy (and wrong with # 2 change above). It should not depend on details of pending state manager, and rather rely on if it has any pending state to offer.
   • Overall, I do not see any benefits or improvements from not returning "empty" pending state (i.e. undefined, as opposed to defined but empty). It helps remove another assumption / implicit dependency (logic in Container.ts returning {} when in fact it does not know anything about type of that field.
4. I changed logic in applyStashedOpsAt() to make it on-par with previous logic (as it would otherwise fail "snapshot was refreshed" test after all above changes), but I believe the current logic has serious bug. I do not see any communication between container runtime and loader layer communicating that potential snapshot is safe to use. I.e. that its sequence number is not higher than lowest reference sequence number across pending ops. If it refreshes too far, and there is no reconnect to socket (that forces op rebasing), then next serialization could produce broken state as it will make it impossible to apply stashed ops.
5. Mostly unrelated to all above: I do not see us propagating savedOp metadata property from grouped ops (as visible in Container) to ungrouped ops. OpGroupingManager.ungroupOp() does not do it as far as I can see. I think we are getting lucky with ID compression ops as they are usually not grouped (there is usually a single ID compression op, as it's part of its own batch). When/if any of those assumptions change, this code would stop working.
   • Propagate properly this property.

[msfluid-bot]

⯅ @fluid-example/bundle-size-tests: +435 Bytes

Metric Name	Baseline Size	Compare Size	Size Diff
aqueduct.js	452.76 KB	452.89 KB	⯅ +132 Bytes
azureClient.js	549.21 KB	549.31 KB	⯅ +101 Bytes
connectionState.js	680 Bytes	680 Bytes	■ No change
containerRuntime.js	256.01 KB	256.14 KB	⯅ +132 Bytes
fluidFramework.js	339.57 KB	339.57 KB	■ No change
loader.js	133.01 KB	132.98 KB	⯆ -31 Bytes
map.js	41.42 KB	41.42 KB	■ No change
matrix.js	143.41 KB	143.41 KB	■ No change
odspClient.js	517.09 KB	517.19 KB	⯅ +101 Bytes
odspDriver.js	97.06 KB	97.06 KB	■ No change
odspPrefetchSnapshot.js	41.93 KB	41.93 KB	■ No change
sharedString.js	161.19 KB	161.19 KB	■ No change
sharedTree.js	339.57 KB	339.57 KB	■ No change
Total Size	3.15 MB	3.15 MB	⯅ +435 Bytes

---

Baseline commit: fb1c1da

Generated by 🚫 dangerJS against edbffb4

[vladsud]

FYI - Looks like this PR conflicts with #20427, and possibly some of the issues raised in this PR are addressed by it.

[dannimad]

Hi Vlad. I think most of your concerns were solved in the PR you mentioned above. Once we stopped dropping the clientID, all sort of bugs and fishy behavior surfaced, which were solved in that PR. Let me know if you still have concerns about points 1 through 4, since 5 most likely hasn't been fixed.

[vladsud]

Forked savedOp changed into #20837.

[vladsud]

Closing in favor of

• Update conditions to refresh the snapshot and sessionExpiryTimer #20427
• Filter pending state manager ops while getLocalState by sequence number. #20846
• Fix "savedOp" metadata property propagation for grouped ops #20837

Note - I made a mistake on Line 206 in pendingStatemanager.ts in this PR. Comparison should have been (for "initial ops") based on sequence number! Same issue as in first PR - thinking in sequence numbers is hard!