feat: Handle database connection errors with 500 #725
Conversation
chriswk
force-pushed
the
database-disconnect-handling
branch
from
3c9d5d7
to
f322333
Compare
| fields, | ||
| ); | ||
| res.json({ version, features }); | ||
| try { |
There was a problem hiding this comment.
would it make sense to move all these "try/catch" calls to the "base-controller" class?
(seems like we are always doing the same thing)
There was a problem hiding this comment.
That would then probably mean adding a error-handling-middleware, cause the controller base class method doesn't actually have access to req or res
There was a problem hiding this comment.
Let leave it like this for now then. Could be that the controller should have access to req/res, on the other hand, it then starts to smell like a custom middleware.
There was a problem hiding this comment.
Well, seeing as how we're calling the same util/handleErrors method for almost all of them, writing a custom error-handling middleware might be just what we want.
src/lib/services/addon-service.js
Outdated
| return addons; | ||
| } catch (e) { | ||
| this.logger.warn(`Could not get addons ${e}`); | ||
| return []; |
There was a problem hiding this comment.
I would actually argue that we should store the addon configurations and return the cached / stale once if we are unable to fetch the configs. Now we are ending up removing them, and not doing "addon" logic in the case the db is removed. It can lead to lost messages to a webhook (because of a temporary db-error).
There was a problem hiding this comment.
How would we store them, what does memoizee do if we don't return anything from a call instead of the empty list?
There was a problem hiding this comment.
Need to check the docs. I do not think memoizee do anything clever automatically.
There was a problem hiding this comment.
I see errors are not cached, but it also seems to void the cache for the key that fails. So it wasn't enough to simply not have error handling inside the memoizee wrapper
There was a problem hiding this comment.
Seems like it already does what we want. Does not cache a promise that errors, but does not crash the application either, it simply retries the wrapped function the next time we need the data. I reverted to how it looked on master for just this
There was a problem hiding this comment.
Which isn't what we want. We do want to keep the stale config. Hmm, let's discuss this tomorrow
chriswk
force-pushed
the
database-disconnect-handling
branch
from
f322333
to
4ae7700
Compare
- If database goes away while unleash is running, unleash now stays running, but all api endpoints will return 500. - This includes our health endpoint, which allows k8s or similar orchestrators to decide what should be done, rather than Unleash terminating unexpectedly
chriswk
force-pushed
the
database-disconnect-handling
branch
from
4ae7700
to
b077a20
Compare
Bumps [nanoid](https://github.com/ai/nanoid) from 3.1.28 to 3.3.1. - [Release notes](https://github.com/ai/nanoid/releases) - [Changelog](https://github.com/ai/nanoid/blob/main/CHANGELOG.md) - [Commits](ai/nanoid@3.1.28...3.3.1) --- updated-dependencies: - dependency-name: nanoid dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
running, but all api endpoints will return 500.
orchestrators to decide what should be done, rather than Unleash
terminating unexpectedly