-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
feat(api): invalidate stale workflows #6887
Conversation
|
||
for (const identifier of staleWorkflowIdentifiers) { | ||
await this.invalidateCacheService.invalidateByKey({ | ||
key: buildNotificationTemplateIdentifierKey({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The update-workflow.command that is handling existing workflows already invalidates the cache for both keys - See here.
I suggest we apply a more surgical solution to avoid fetching working multiple times from the database that adds extra DB requests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You make a good point. The data that gets validated on updates should indeed be part of what we check.
To illustrate today's issue, here’s the scenario we need to address:
- A trigger is sent to a non-existent workflow with ID "best-workflow." On the trigger, we try to fetch the workflow, but since it doesn’t exist, we store
null
in the cache. - We then sync "best-workflow."
- When we trigger "best-workflow" again, we pull the
null
value from the cache.
To resolve this, we could prevent null
from being cached on creation.
While reviewing the issue, I thought we had another problem, but I realized it's not a bug—just an instance of stale memory that we could release. Now, I’m considering removing the extra cache invalidation step and relying on the TTL to clear it, which would simplify the logic and reduce the load on Redis.
What do you think?
✅ Deploy Preview for novu-stg-vite-dashboard-poc ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
}) | ||
) | ||
); | ||
const invalidateCachesByIdentifiersPromises = workflowsToDelete.map((workflow) => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This invalidation should be added in the delete-workflow usecase. The cache clearance should applies every time a workflow is deleted, not only via sync.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moreover, the delete-workflow use case needs to delete from the preferences collection all entries of type WORKFLOW_RESOURCE
by _environmentId and workflow internal id. This is irrelevant to this bug, but as I reviewed the code, it's an omission we can easily tackle.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
I deleted the old bridge.deleteWorkflow so we could reuse one reusable delete workflow from app-gen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great clean up @djabarovgeorge 👏 👏 👏
What changed? Why was the change needed?
Screenshots
Expand for optional sections
Related enterprise PR
Special notes for your reviewer