Remove serverless functions on soft delete

[martmull]

Delete workflow versions serverless functions when soft delete workflow

[greptile-apps]

PR Summary

This PR implements serverless function cleanup when workflows are soft-deleted, ensuring proper resource management across the system.

• Added ServerlessFunctionModule import in workflow-query-hook.module.ts to enable serverless function cleanup
• Modified cleanWorkflowsSubEntities in workflow-common.workspace-service.ts to accept workspaceId parameter for scoped operations
• Added deleteServerlessFunctions method in workflow-common.workspace-service.ts for handling serverless function cleanup
• Updated post-query hooks to pass workspace ID from auth context to cleanup methods

_{4 file(s) reviewed, 2 comment(s)
Edit PR Review Bot Settings | Greptile}

[greptile-apps]

logic: Double nested Promise.all/map with async callback will not wait for serverless function deletions to complete. Array.map() with async doesn't return promises that can be awaited

[Devessier]

greptile is kinda true here.

Promise.all expects an array of Promise. Here, we give an Array<Array<Promise>>. If we want to wait for all the promises to be fulfilled, we must flatten the array with Array#flat.

Furtheremore, calling Promise.all is only useful if you want to wait for a set of promises to be fulfilled. Here, we don't wait for this, the function immediately returns—the Promise.all is not awaited. We can get rid of it.

The problem I can see here is that any failing promise won't be caught. I couldn't find any code snippet in the codebase where we do something similar, but I would do:

  workflowVersions.forEach((workflowVersion) => {
    workflowVersion.steps?.forEach((step) => {
      const cb = () => {
        if (step.type === WorkflowActionType.CODE) {
          await this.serverlessFunctionService.deleteOneServerlessFunction(
            step.settings.input.serverlessFunctionId,
            workspaceId,
          );
        }
      }

      cb().catch(Logger.catch)
    });
  })

[Devessier]

Uncaught promise rejections make a Node.js process fail. According to my tests, we have a global listener to prevent such crashes, but I would still catch the errors explicitly to provide more context about the error.

As I said, I couldn't see where we did something similar, so we might first want to discuss it broadly with the team.

[Devessier]

I wanted to open a discussion about some topics. I'm unsure the team agreed on a pattern here.

[Devessier]

greptile is kinda true here.

Promise.all expects an array of Promise. Here, we give an Array<Array<Promise>>. If we want to wait for all the promises to be fulfilled, we must flatten the array with Array#flat.

Furtheremore, calling Promise.all is only useful if you want to wait for a set of promises to be fulfilled. Here, we don't wait for this, the function immediately returns—the Promise.all is not awaited. We can get rid of it.

The problem I can see here is that any failing promise won't be caught. I couldn't find any code snippet in the codebase where we do something similar, but I would do:

  workflowVersions.forEach((workflowVersion) => {
    workflowVersion.steps?.forEach((step) => {
      const cb = () => {
        if (step.type === WorkflowActionType.CODE) {
          await this.serverlessFunctionService.deleteOneServerlessFunction(
            step.settings.input.serverlessFunctionId,
            workspaceId,
          );
        }
      }

      cb().catch(Logger.catch)
    });
  })

[Devessier]

Uncaught promise rejections make a Node.js process fail. According to my tests, we have a global listener to prevent such crashes, but I would still catch the errors explicitly to provide more context about the error.

As I said, I couldn't see where we did something similar, so we might first want to discuss it broadly with the team.

[Devessier]

Perfect!