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.

Sign up for GitHub

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

Jump to bottom

Server crash if 500 is thrown #8607

Closed
guillim opened this issue Nov 20, 2024 · 6 comments · Fixed by #8835
Closed

Server crash if 500 is thrown #8607

guillim opened this issue Nov 20, 2024 · 6 comments · Fixed by #8835
Assignees
@guillim
Labels
prio: high

Comments

@guillim
Copy link
Contributor

guillim commented Nov 20, 2024

Bug Description

Context

I found out this issue locally running on macOS the command npx nx run twenty-server:start
I was trying to sync emails on a setup where the google env var were not defined.

IS_SIGN_UP_DISABLED=false
# AUTH_MICROSOFT_ENABLED=false
# AUTH_MICROSOFT_CLIENT_ID=replace_me_with_azure_client_id
# AUTH_MICROSOFT_TENANT_ID=replace_me_with_azure_tenant_id
# AUTH_MICROSOFT_CLIENT_SECRET=replace_me_with_azure_client_secret
# AUTH_MICROSOFT_CALLBACK_URL=http://localhost:3000/auth/microsoft/redirect
# AUTH_GOOGLE_ENABLED=true
# AUTH_GOOGLE_CLIENT_ID=replace_me_with_google_client_id
# AUTH_GOOGLE_CLIENT_SECRET=replace_me_with_google_client_secret
# AUTH_GOOGLE_CALLBACK_URL=http://localhost:3000/auth/google/redirect
# AUTH_GOOGLE_APIS_CALLBACK_URL=http://localhost:3000/auth/google-apis/get-access-token
# AUTH_SSO_ENABLED=true

Image

The bug

The server crashes, and no other request can be handled by the server until it is manually restarted.

/Users/gl/Documents/twenty/packages/twenty-server/src/engine/core-modules/auth/filters/auth-rest-api-exception.filter.ts:26
        throw new UnauthorizedException(exception.message);
              ^
UnauthorizedException: Google apis auth is not enabled
    at AuthRestApiExceptionFilter.catch (/Users/gl/Documents/twenty/packages/twenty-server/src/engine/core-modules/auth/filters/auth-rest-api-exception.filter.ts:26:15)
    at ExceptionsHandler.invokeCustomFilters (/Users/gl/Documents/twenty/node_modules/@nestjs/core/exceptions/exceptions-handler.js:33:26)
    at ExceptionsHandler.next (/Users/gl/Documents/twenty/node_modules/@nestjs/core/exceptions/exceptions-handler.js:13:18)
    at /Users/gl/Documents/twenty/node_modules/@nestjs/core/router/router-proxy.js:13:35
    at processTicksAndRejections (node:internal/process/task_queues:95:5)```

Expected behavior

  • First of all, we should not display the option to sync email with google in this situation where the env var are missing.
  • Second, when the server handles this 500 error, it should catch it and return a more appropriate message.
  • Finally, the server should not freeze like that.
  • Bonus, we could keep track of these errors in sentry

To be discussed further with @charlesBochet
@charlesBochet
Copy link
Member

Bumping this one to prio high

@myspace20
Copy link

I will like to work on this issue.

@guillim
Copy link
Contributor Author

guillim commented Nov 25, 2024

Did you find out anything @myspace20 ?

@myspace20
Copy link

I was a little busy. I will find some time to look at it again.

@guillim guillim self-assigned this Nov 25, 2024
@guillim
Copy link
Contributor Author

guillim commented Nov 25, 2024

Technical suggestion

  • Catch this specific 500 error
  • Make sure catched 500 errors are sent to sentry for the Cloud version
  • Hide the option to sync email with google in this situation where the according env var is missing

FurtherMore

  • Investigate why server crashes entirely for this kind of error

@charlesBochet charlesBochet moved this from 🆕 New to 🔖 Planned in Product development ✅ Nov 25, 2024
@guillim guillim moved this from 🔖 Planned to 🏗 In Progress in Product development ✅ Dec 2, 2024
@guillim guillim mentioned this issue Dec 2, 2024
Merged
4 tasks
@guillim
Copy link
Contributor Author

guillim commented Dec 2, 2024 •
edited
Loading

From what I see, the server crash happens with AuthException errors.
I could reproduce with these items

  • InternalServerErrorException: Token has expired.
  • UnauthorizedException: Google apis auth is not enabled

However, If i manually throw an AuthExceptionCode.CLIENT_NOT_FOUND error (same class AuthException) from the GraphqlQueryFindOneResolverService then it is catched properly

My guess : Yoga catches properly the error. We need to find out who calls the other errors. Suspect :

  • @nestjs/core/router/router-proxy.js

Edit :
after the first renaming of Errors, and little refacto, I cannot reproduce the error.

@Weiko Weiko closed this as completed in 7e4277f Dec 3, 2024
@github-project-automation github-project-automation bot moved this from 🏗 In Progress to ✅ Done in Product development ✅ Dec 3, 2024
@guillim guillim moved this from ✅ Done to 🏗 In Progress in Product development ✅ Dec 4, 2024
@guillim guillim moved this from 🏗 In Progress to ✅ Done in Product development ✅ Dec 4, 2024
mdrazak2001 pushed a commit to mdrazak2001/twenty that referenced this issue Dec 4, 2024
@guillim @mdrazak2001
Better errors handling (twentyhq#8835)
5f90db9 
- [ ] Catch this specific `500` error
- [ ] Make sure catched `500` errors are sent to sentry for the Cloud
version
- [ ] Hide the option to sync email with google in this situation where
the according env var is missing
- [x] Add Worskpace information to all catched errors for better
debugging

fix twentyhq#8607
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@guillim guillim

Labels
prio: high
Projects
Product development ✅
Status: ✅ Done
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Better errors handling twentyhq/twenty
3 participants
@charlesBochet @guillim @myspace20