Clean up stale local references referring to old interval endpoints

[jzaffiro]

During the interval change codepath, many local references are created and updated, but local references for removed interval endpoints were never cleaned up. This was causing a bug where after removing a range of text and reverting the remove, the interval endpoints would not return to the position at which they were created. Some existing unit tests also used the stale references, and these only worked in the past because we didn't do any clean up. Those tests now use the updated references.

AB#7044, #19352

[msfluid-bot]

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

Metric Name	Baseline Size	Compare Size	Size Diff
aqueduct.js	514.68 KB	514.79 KB	⯅ +119 Bytes
connectionState.js	680 Bytes	680 Bytes	■ No change
containerRuntime.js	249.11 KB	249.11 KB	■ No change
loader.js	128.15 KB	128.15 KB	■ No change
map.js	41.36 KB	41.36 KB	■ No change
matrix.js	143.67 KB	143.67 KB	■ No change
odspDriver.js	97.5 KB	97.5 KB	■ No change
odspPrefetchSnapshot.js	41.87 KB	41.87 KB	■ No change
sharedString.js	161.26 KB	161.38 KB	⯅ +119 Bytes
sharedTree.js	330.25 KB	330.25 KB	■ No change
Total Size	1.81 MB	1.81 MB	⯅ +238 Bytes

---

Baseline commit: 023de01

Generated by 🚫 dangerJS against e55b6aa

[anthony-murphy]

It still feels a bit odd to me that this change would affect anything, or only affect the right things. Specifically, I'm not sure why this change doesn't need to consider if the change is made locally vs remotely. Usually local changes are optimistically applied, and then the real change is commit/finalized/completed on ack. That said, interval collections have a pretty odd flow for dealing with ops and acks plus i know we have pretty good testing here with fuzz, what's added here, and otherwise, so i trust we are doing the right things overall.

[scarlettjlee]

There doesn't seem to be anything wrong with this change itself, but I still don't understand how/why it fixes the bug which makes me confused and wondering if there are more lurking bugs.

[scarlettjlee]

The fix is good. The root problem is the logic in addIfIntervalEndpoint: it looks for any interval in the segments it examines, not current interval collections. It should probably validate that as well, but we can do that later. We want to clean up these unnecessary local refs anyway. (There may also be another lurking problem with extra change events on reverting, but that's a separate issue.)