GC back compat - Add flag in summary to allow/disallow

[arinwt]

Right now it works like so: the flag on the metadata (in the summary) will specify a preference whether or not GC is allowed. True to enable GC, false to disable it, or undefined to leave the decision totally up to the runtime.

Behavior is as follows (for now):

gcAllowed	gcFeature	disableGC	runGC
true	1	true	false ❌
true	1	false	true ✔
true	1	undefined	true ✔
false/undefined	0	true	false ❌
false/undefined	0	false	false ❌
false/undefined	0	undefined	false ❌

Right now there is no way to change this value after it is initially set.

For documents earlier than summaryFormatVersion == 1 (i.e. if they don't have the metadata blob yet), then we assume gcFeature is explicitly 0 which means disallowed, and will never runGC.

To set it the first time, just pass the desired value in runtimeOptions as gcAllowed. If nothing is passed to this runtime option, then it defaults to false and will stay that way. When serialized true -> 1 for allowed, and false -> 0 for disallowed. In the future we may repurpose the 1+ to be an incrementing version for GC.

Fixes #4948

[msfluid-bot]

■ @fluidframework/base-host: No change

Metric Name	Baseline Size	Compare Size	Size Diff
main.js	183.1 KB	183.1 KB	■ No change
Total Size	183.1 KB	183.1 KB	■ No change

⯅ @fluid-example/bundle-size-tests: +1.09 KB

Metric Name	Baseline Size	Compare Size	Size Diff
container.js	201.34 KB	202.43 KB	⯅ +1.09 KB
map.js	49.11 KB	49.11 KB	■ No change
matrix.js	143.42 KB	143.42 KB	■ No change
odspDriver.js	201.22 KB	201.22 KB	■ No change
sharedString.js	158.14 KB	158.14 KB	■ No change
Total Size	753.24 KB	754.33 KB	⯅ +1.09 KB

---

Baseline commit: 30d6b5d

Generated by 🚫 dangerJS against ae62ce3

[anthony-murphy]

can we put all the gc options into a sub-object, rather than having them all directly on IContainerRuntimeOptions. seems to be sprawling a bit

[arinwt]

I broke into summaryOptions and gcOptions. Let me know what you think.

[anthony-murphy]

I don't like this as a member function of this class, and i don't like the value changing at runtime, we should use this data to build the initial configuration, and then only depend on the config, two sources of truth like this always leads to bugs

[arinwt]

I agree with you.
I changed to readonly properties: shouldRunGC and disableIsolatedChannels. Now I no longer modify the passed in runtimeOptions for those.

I kept the shouldWriteMetadata getter, because it is a computed property using all readonly properties.

[anthony-murphy]

i don't like that enableGc means one thing here, and somethings else on the container, please unify the config names

[arinwt]

I don't know how much it would help, but maybe I can change to summaryAllowGC ?
summary - to indicate it is per document / stored in summary
allow - vs. enable to show that this is less of an option more of a restriction?

@agarwal-navin also thoughts?

[agarwal-navin]

I am wondering if having both enableGC / summaryEnableGC and disableGC is a bit too much. It is confusing for sure. We can probably just have enableGC and the override (to disable it) can be done via the local storage or via loader options (which would be hidden).

[agarwal-navin]

If there is metadata blob but it does not have enableGC property in it, this will default to running GC right. I am wondering if that is the behavior we want.
What will happen in this scenario - A document is created before the "GC waterline" and it does not provide the summaryEnableGC runtime option. But we write the metadata blob for it (maybe because of disableIsolatedChannels?) then we will end up running GC on this document right?

[arinwt]

Yes I thought that was correct (since we are in v1+), but now I have changed it to always write false for now.

[agarwal-navin]

We need tests to verify that we do enable / disable GC correctly based on different combination of these options.

We can probably add more combinations to the tests I have in this PR - #5539.

[vladsud]

Couple mete-observations around GC and similar features:

1. Overall, I'd love to see a way for enabling new features for new files only, as a way of staging. I.e. if I want to roll new functionality, it's beneficial to leave existing files as is (either forever, or come back to enable feature later for them) and just enable for new files. This would require some property bag associated with a file (and easy access to it from any place in runtime, and ideally - driver as well), such that a feature could test property from that bag and determine if feature is enabled

   • Someone can use external logic (like partition based on SPO audience) to enable features on sub-set of files
   • Blob aggregation is great example of feature like that. Enabling it only for new files ensures that damage I can cause (in case something goes wrong) is rather small, and disabling it as a recovery mechanism will lose some number of files, but likely nothing that is already used in prod, which is great.

2. But when it comes to GC, boolean will not work. It's almost guaranteed that we will find bugs in GC (overall, let's say some component does not use handles). I.e. after we enabled it and set a flag, we may find ourselves in situation where files are not correctly represented in search index. I see couple outcomes

   1. Overall, we do not care, and just say - files created at GA+ are correct, everything else is best effort. In this case we do not care about boolean at all.
   2. We need not boolean, but counter / version. Every time we find that version X has a bug, we fix it, bump version and tell storage to ignore all files with <=X version.
   3. we use current approach. It's a mix of two approaches above, because I'm sure we will get it wrong and boolean will be just non-perfect filter, but GA (and files created at GA) would be the real test / filter.

Note that for GC, we would love to enable as much of GC pipeline as quickly as possible, so I think waiting enabling 2.3 is not great. I'd use 2.2 and maybe use runtime version (that created a file) as a poor man's indicator of GC versions. Storage may want to play this game with us, or maybe not and just index it all the time based on GC data, at which case it's 2.1, but we have our own telemetry to say "oh yes, that's an old file, it's expected it's not perfect when it comes to GC"

Note that if we find bugs after GA, having version in the file (that created a file) might be handy - we might write some scripts that run in storage, find these files and fix them. This is super expensive, so we will likely do it only if absolutely required, but we should leave this option available as a safety net.

[arinwt]

@vladsud - let me know if I got something wrong here. The suggestion is to include the runtime version in the metadata blob, and use it to map to "GC version", so we know which version of GC we are on, and thus which files we need to exclude/filter as "bad"? (2.2 above)

[arinwt]

I can also imagine the property bag you mention in 1, but it just has the version for each feature, or 0/null/undefined if it is disabled. And it is stored in the metadata blob.

gcFeature: 0 | 1 | 2 - 0 is disabled, 1 is first version, 2 is second version, etc.
blobAggregationFeature: 0 | 1 - 0 is disabled, 1 is enabled

[arinwt]

Hey @vladsud, after some discussion, I think we can switch to a 0/1 number type instead of a boolean, but keep everything else the same. It feels like the example of app using handles is more of an app-problem, and would have nothing to do with a GC version from the runtime's perspective. Let me know if you see any problem with this.