Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(types): fix type cache crash #3841

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

odbol
Copy link

@odbol odbol commented Nov 7, 2024

There's some kind of weird race condition or something where the strict null check fails but then returns an undefined object.

See firebase/genkit#972 for details on how to reproduce.

  name: 'TypeError',
  message: "Cannot destructure property 'shape' of 'this._getCached(...)' as it is undefined.",
  stack: "TypeError: Cannot destructure property 'shape' of 'this._getCached(...)' as it is undefined.\n" +
    '    at ZodObject._parse (/Users/fuego/Documents/development/gemineye-server/node_modules/zod/lib/types.js:1848:17)\n' +
    '    at ZodObject._parseSync (/Users/fuego/Documents/development/gemineye-server/node_modules/zod/lib/types.js:146:29)\n' +
    '    at /Users/fuego/Documents/development/gemineye-server/node_modules/zod/lib/types.js:1711:29\n' +
    '    at Array.map (<anonymous>)\n' +
    '    at ZodArray._parse (/Users/fuego/Documents/development/gemineye-server/node_modules/zod/lib/types.js:1710:38)\n' +
    '    at ZodObject._parse (/Users/fuego/Documents/development/gemineye-server/node_modules/zod/lib/types.js:1864:37)\n' +
    '    at ZodObject._parseSync (/Users/fuego/Documents/development/gemineye-server/node_modules/zod/lib/types.js:146:29)\n' +
    '    at ZodObject.safeParse (/Users/fuego/Documents/development/gemineye-server/node_modules/zod/lib/types.js:176:29)\n' +
    '    at ZodObject.parse (/Users/fuego/Documents/development/gemineye-server/node_modules/zod/lib/types.js:157:29)\n' +
    '    at /Users/fuego/Documents/development/gemineye-server/node_modules/@genkit-ai/flow/lib/flow.js:522:55',

There's some kind of weird race condition or something where the strict null check fails but then returns an undefined object. 

See firebase/genkit#972 for details on how to reproduce.

```
  name: 'TypeError',
  message: "Cannot destructure property 'shape' of 'this._getCached(...)' as it is undefined.",
  stack: "TypeError: Cannot destructure property 'shape' of 'this._getCached(...)' as it is undefined.\n" +
    '    at ZodObject._parse (/Users/fuego/Documents/development/gemineye-server/node_modules/zod/lib/types.js:1848:17)\n' +
    '    at ZodObject._parseSync (/Users/fuego/Documents/development/gemineye-server/node_modules/zod/lib/types.js:146:29)\n' +
    '    at /Users/fuego/Documents/development/gemineye-server/node_modules/zod/lib/types.js:1711:29\n' +
    '    at Array.map (<anonymous>)\n' +
    '    at ZodArray._parse (/Users/fuego/Documents/development/gemineye-server/node_modules/zod/lib/types.js:1710:38)\n' +
    '    at ZodObject._parse (/Users/fuego/Documents/development/gemineye-server/node_modules/zod/lib/types.js:1864:37)\n' +
    '    at ZodObject._parseSync (/Users/fuego/Documents/development/gemineye-server/node_modules/zod/lib/types.js:146:29)\n' +
    '    at ZodObject.safeParse (/Users/fuego/Documents/development/gemineye-server/node_modules/zod/lib/types.js:176:29)\n' +
    '    at ZodObject.parse (/Users/fuego/Documents/development/gemineye-server/node_modules/zod/lib/types.js:157:29)\n' +
    '    at /Users/fuego/Documents/development/gemineye-server/node_modules/@genkit-ai/flow/lib/flow.js:522:55',
```
Copy link

netlify bot commented Nov 7, 2024

Deploy Preview for guileless-rolypoly-866f8a ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit f281576
🔍 Latest deploy log https://app.netlify.com/sites/guileless-rolypoly-866f8a/deploys/672d1f1df89cea0008372d19
😎 Deploy Preview https://deploy-preview-3841--guileless-rolypoly-866f8a.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@@ -2421,7 +2421,7 @@ export class ZodObject<
private _cached: { shape: T; keys: string[] } | null = null;

_getCached(): { shape: T; keys: string[] } {
if (this._cached !== null) return this._cached;
if (this._cached) return this._cached;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As you can see the type above is {shape:T;keys:string[]} or null, with a default to null.

So this questions as to why the null check is not correct?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah it's definitely weird. Seems like it shouldn't happen, at least according to Typescript. Is it possible that somehow shape and keys could end up undefined? Would that cause _cached to be undefined?

I'm not sure how this is happening, if it's some kind of node.js interpreter error or race condition, or some weird edge case that Genkit just ran across... but this patch does fix it at least, and seems still correct. What do you think we should do?

@odbol
Copy link
Author

odbol commented Nov 14, 2024

Hey @m10rten, any thoughts on next steps here? This bug is blocking our launch, and I'm not sure how else to fix it...

@m10rten
Copy link
Contributor

m10rten commented Nov 15, 2024

Hey @m10rten, any thoughts on next steps here? This bug is blocking our launch, and I'm not sure how else to fix it...

Hi, suggest to use a type cast for the time being as when this gets merged you wont be able to use it till its released.

@odbol
Copy link
Author

odbol commented Nov 20, 2024

Hey @m10rten, any thoughts on next steps here? This bug is blocking our launch, and I'm not sure how else to fix it...

Hi, suggest to use a type cast for the time being as when this gets merged you wont be able to use it till its released.

hmmm... I'm not sure how to use a typecast in this situation. The use of Zod is pretty heavily buried into the Genkit library I'm using; I don't think their API lets us get around that.

But are you're saying that this fix does have a chance of getting merged? Any ETA on when the next release will be?

youre telling me theres a chance

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants