Stop using short data store IDs in detached container #21613
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
agarwal-navin
requested review from
vladsud,
markfields,
a team,
pragya91,
jatgarg,
tyler-cai-microsoft,
kian-thompson and
rajatch-ff
and removed request for
a team
github-actions
bot
added
area: runtime
vladsud
reviewed
Jun 25, 2024
| * | ||
| * While we figure out the fix, we are disabling the ability to create short IDs and this assert validates it. | ||
| */ | ||
| function assertInvert(value: unknown, message: string) { |
There was a problem hiding this comment.
Why it's unknown, not boolean type? All the cases seem to be boolean.
There was a problem hiding this comment.
Changed it to boolean
vladsud
reviewed
Jun 25, 2024
| assert(typeof idTest === "number", "short IDs should be issued"); | ||
| assert(idTest >= 0, "finalId"); | ||
| assertInvert(typeof idTest === "number", "short IDs should be issued"); | ||
| // assert(idTest >= 0, "finalId"); |
There was a problem hiding this comment.
Maybe be roll it into previous assert? I.e.
assertInvert(typeof idTest === "number" && idTest >= 0, "short IDs should be issued");
vladsud
reviewed
Jun 25, 2024
| const dataStore1 = | ||
| await defaultDataStore.context.containerRuntime.createDataStore(TestDataObjectType); | ||
| const ds1 = (await dataStore1.entryPoint.get()) as ITestFluidObject; | ||
| assertInvert( |
There was a problem hiding this comment.
Should we change this test and create 100 of detached data stores?
There was a problem hiding this comment.
We could but I don't see a point. Are you suggesting we the load a container and basically validate that it can load all these data stores, process ops, etc?
There was a problem hiding this comment.
Yeah you need 13 to create a datastore with id "["
There was a problem hiding this comment.
Sure, but there is no repro of the issue even with that. So, I don't see a point in doing it yet. Once we have a repro, we can add test for this.
There was a problem hiding this comment.
vladsud
approved these changes
Jun 25, 2024
rajatch-ff
reviewed
Jun 25, 2024
| this.dataStoreContext.containerRuntime.generateDocumentUniqueId?.() ?? uuid(); | ||
| id = typeof res === "number" ? encodeCompactIdToString(2 * res + 1, "_") : res; | ||
| } | ||
| // if (this.visibilityState !== VisibilityState.GloballyVisible) { |
There was a problem hiding this comment.
Should we put all of these changes behind a feature flag, for easy toggles in DF/PROD?
There was a problem hiding this comment.
I am not sure I see the value in doing that because we can't enable it unless we fix the bug which will require making code changes anyway. We can enable it via that change and add feature flags if needed.
There was a problem hiding this comment.
I hear you there. Feature flag might come handy, if the issue happens to be on ODSP side, and we can just toggle our change off without making a code change. Since the true-root-cause is still being figured out, there may be value in keeping this change behind a switch.
There was a problem hiding this comment.
I can add it behing a feature. But I doubt we will use it (Loop would probably not let us it being so risky). Also, we heard from ODSP that it's not their issue most likely.
Let me work on adding a feature, it doesn't hurt :).
There was a problem hiding this comment.
Added feature flags
|
Any side-effects of turning off short-ids that we should know about or consider before merging? |
⯅ @fluid-example/bundle-size-tests: +806 Bytes
Baseline commit: d2ba7eb |
markfields
changed the title
AFAIK, there shouldn't be. I will defer to @vladsud to double check. |
|
We are not attempting to recover existing documents. Change impacts new documents only (it will make them work again). |
rajatch-ff
approved these changes
Jun 25, 2024
agarwal-navin
added a commit
to agarwal-navin/FluidFramework
that referenced
this pull request
Jun 25, 2024
Currently, data stores created in detached container always get a short ID via id compressor. We have a bug in one of our customer's app where a short data store ID created is `[` but in a downloaded snapshot, it is converted to its ASCII equivalent `%5B` in certain conditions. So, when an op comes for this data store with id `[`, containers loaded with this snapshot cannot find the data store. We haven't yet identified what causes the id to change from `[` -> `%5B`. However, we know that it is caused by short IDs generated in detached container. To mitigate the issue for customers, this PR disables the generation of short IDs for data stores and DDSes. Added tests that validate that we don't generate short IDs for data stores and DDSes in detached or attached state.
agarwal-navin
added a commit
to agarwal-navin/FluidFramework
that referenced
this pull request
Jun 25, 2024
## Bug Currently, data stores created in detached container always get a short ID via id compressor. We have a bug in one of our customer's app where a short data store ID created is `[` but in a downloaded snapshot, it is converted to its ASCII equivalent `%5B` in certain conditions. So, when an op comes for this data store with id `[`, containers loaded with this snapshot cannot find the data store. ## Root-cause We haven't yet identified what causes the id to change from `[` -> `%5B`. However, we know that it is caused by short IDs generated in detached container. ## Fix To mitigate the issue for customers, this PR disables the generation of short IDs for data stores and DDSes. Added tests that validate that we don't generate short IDs for data stores and DDSes in detached or attached state.
pragya91
pushed a commit
to pragya91/FluidFramework-1
that referenced
this pull request
Jun 25, 2024
Currently, data stores created in detached container always get a short ID via id compressor. We have a bug in one of our customer's app where a short data store ID created is `[` but in a downloaded snapshot, it is converted to its ASCII equivalent `%5B` in certain conditions. So, when an op comes for this data store with id `[`, containers loaded with this snapshot cannot find the data store. We haven't yet identified what causes the id to change from `[` -> `%5B`. However, we know that it is caused by short IDs generated in detached container. To mitigate the issue for customers, this PR disables the generation of short IDs for data stores and DDSes. Added tests that validate that we don't generate short IDs for data stores and DDSes in detached or attached state.
daesunp
pushed a commit
that referenced
this pull request
Jun 25, 2024
Currently, data stores created in detached container always get a short ID via id compressor. We have a bug in one of our customer's app where a short data store ID created is `[` but in a downloaded snapshot, it is converted to its ASCII equivalent `%5B` in certain conditions. So, when an op comes for this data store with id `[`, containers loaded with this snapshot cannot find the data store. We haven't yet identified what causes the id to change from `[` -> `%5B`. However, we know that it is caused by short IDs generated in detached container. To mitigate the issue for customers, this PR disables the generation of short IDs for data stores and DDSes. Added tests that validate that we don't generate short IDs for data stores and DDSes in detached or attached state. (cherry picked from commit 1c00d7e)
pragya91
pushed a commit
to pragya91/FluidFramework-1
that referenced
this pull request
Jun 26, 2024
## Bug Currently, data stores created in detached container always get a short ID via id compressor. We have a bug in one of our customer's app where a short data store ID created is `[` but in a downloaded snapshot, it is converted to its ASCII equivalent `%5B` in certain conditions. So, when an op comes for this data store with id `[`, containers loaded with this snapshot cannot find the data store. ## Root-cause We haven't yet identified what causes the id to change from `[` -> `%5B`. However, we know that it is caused by short IDs generated in detached container. ## Fix To mitigate the issue for customers, this PR disables the generation of short IDs for data stores and DDSes. Added tests that validate that we don't generate short IDs for data stores and DDSes in detached or attached state.
agarwal-navin
added a commit
that referenced
this pull request
Jun 26, 2024
…21613) (#21628) Porting 1c00d7e to rc 3 branch. ## Bug Currently, data stores created in detached container always get a short ID via id compressor. We have a bug in one of our customer's app where a short data store ID created is [ but in a downloaded snapshot, it is converted to its ASCII equivalent %5B in certain conditions. So, when an op comes for this data store with id [, containers loaded with this snapshot cannot find the data store. ## Root-cause We haven't yet identified what causes the id to change from [ -> %5B. However, we know that it is caused by short IDs generated in detached container. ## Fix To mitigate the issue for customers, this PR disables the generation of short IDs for data stores and DDSes. Added tests that validate that we don't generate short IDs for data stores and DDSes in detached or attached state.
daesunp
added a commit
that referenced
this pull request
Jun 26, 2024
…21613) (#21631) Porting 1c00d7e to rc 2 branch. ## Bug Currently, data stores created in detached container always get a short ID via id compressor. We have a bug in one of our customer's app where a short data store ID created is `[` but in a downloaded snapshot, it is converted to its ASCII equivalent `%5B` in certain conditions. So, when an op comes for this data store with id `[`, containers loaded with this snapshot cannot find the data store. ## Root-cause We haven't yet identified what causes the id to change from `[` -> `%5B`. However, we know that it is caused by short IDs generated in detached container. ## Fix To mitigate the issue for customers, this PR disables the generation of short IDs for data stores and DDSes. Added tests that validate that we don't generate short IDs for data stores and DDSes in detached or attached state. --------- Co-authored-by: Navin Agarwal <45832642+agarwal-navin@users.noreply.github.com>
rohandubal
pushed a commit
that referenced
this pull request
Jun 26, 2024
) (#21629) Porting 1c00d7e to 2.0 branch. ## Bug Currently, data stores created in detached container always get a short ID via id compressor. We have a bug in one of our customer's app where a short data store ID created is [ but in a downloaded snapshot, it is converted to its ASCII equivalent %5B in certain conditions. So, when an op comes for this data store with id [, containers loaded with this snapshot cannot find the data store. ## Root-cause We haven't yet identified what causes the id to change from [ -> %5B. However, we know that it is caused by short IDs generated in detached container. ## Fix To mitigate the issue for customers, this PR disables the generation of short IDs for data stores and DDSes. Added tests that validate that we don't generate short IDs for data stores and DDSes in detached or attached state.
pragya91
added a commit
that referenced
this pull request
Jun 26, 2024
…21613) (#21630) Porting 1c00d7e to RC4.0 branch. ## Bug Currently, data stores created in detached container always get a short ID via id compressor. We have a bug in one of our customer's app where a short data store ID created is [ but in a downloaded snapshot, it is converted to its ASCII equivalent %5B in certain conditions. So, when an op comes for this data store with id [, containers loaded with this snapshot cannot find the data store. ## Root-cause We haven't yet identified what causes the id to change from [ -> %5B. However, we know that it is caused by short IDs generated in detached container. ## Fix To mitigate the issue for customers, this PR disables the generation of short IDs for data stores and DDSes. Added tests that validate that we don't generate short IDs for data stores and DDSes in detached or attached state. --------- Co-authored-by: Navin Agarwal <45832642+agarwal-navin@users.noreply.github.com>
pragya91
added a commit
that referenced
this pull request
Jun 26, 2024
…21613) (#21632) Porting 1c00d7e to rc5 branch. ## Bug Currently, data stores created in detached container always get a short ID via id compressor. We have a bug in one of our customer's app where a short data store ID created is `[` but in a downloaded snapshot, it is converted to its ASCII equivalent `%5B` in certain conditions. So, when an op comes for this data store with id `[`, containers loaded with this snapshot cannot find the data store. ## Root-cause We haven't yet identified what causes the id to change from `[` -> `%5B`. However, we know that it is caused by short IDs generated in detached container. ## Fix To mitigate the issue for customers, this PR disables the generation of short IDs for data stores and DDSes. Added tests that validate that we don't generate short IDs for data stores and DDSes in detached or attached state. Co-authored-by: Navin Agarwal <45832642+agarwal-navin@users.noreply.github.com>
rohandubal
pushed a commit
that referenced
this pull request
Jun 26, 2024
## Bug Currently, data stores created in detached container always get a short ID via id compressor. We have a bug in one of our customer's app where a short data store ID created is `[` but in a downloaded snapshot, it is converted to its ASCII equivalent `%5B` in certain conditions. So, when an op comes for this data store with id `[`, containers loaded with this snapshot cannot find the data store. ## Root-cause We haven't yet identified what causes the id to change from `[` -> `%5B`. However, we know that it is caused by short IDs generated in detached container. ## Fix To mitigate the issue for customers, this PR disables the generation of short IDs for data stores and DDSes. Added tests that validate that we don't generate short IDs for data stores and DDSes in detached or attached state.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Bug
Currently, data stores created in detached container always get a short ID via id compressor. We have a bug in one of our customer's app where a short data store ID created is
[but in a downloaded snapshot, it is converted to its ASCII equivalent%5Bin certain conditions. So, when an op comes for this data store with id[, containers loaded with this snapshot cannot find the data store.Root-cause
We haven't yet identified what causes the id to change from
[->%5B. However, we know that it is caused by short IDs generated in detached container.Fix
To mitigate the issue for customers, this PR disables the generation of short IDs for data stores and DDSes. Added tests that validate that we don't generate short IDs for data stores and DDSes in detached or attached state.