search

SeedCompany / cord-api-v3

Bible translation project management API
MIT License
18 stars 4 forks source link

[Hotfix] Change `Promise.all` to `for await` in project status handler to catch UnauthorizedException #3213

Closed andrewmurraydavid closed 4 months ago

andrewmurraydavid commented 4 months ago

Cord API memory leak causing AWS crash

Description

This PR changes how the engagements are updated in the project handler. There seems to be an issue, either with the the way Promise.all handles the UnauthorizedException for the edgecase where a project has 3 engagements, one (or more) of them being in a step previous (for example 2 in Active change plan and 1 in Discussing change plan), the engagement update would fail with the UnauthorizedException.

It is assumed that because Promis.all not handling the exception properly, would cause a neo4j transaction creation to retry in an infinite loop.

This infinite loop caused the API service to crash completely being OOM (out of memory).

Steps to reproduce the bug

In the description

Expected behavior

Not crash, lol

Ready for review checklist

Use [N/A] if the item is not applicable to this PR or remove the item

  • [x] Change the task url above to the actual Monday task
  • [x] Add reviewers to this PR
  • [x] THIS IS A HOTFIX
CarsonF commented 4 months ago

UnauthorizedExceptions should not be treated as retryable. But maybe it was the error from the other queries? I don't think this is the root solution, other queries in parallel could trigger this symptom too. But I'm pretty sure EdgeDB doesn't even let you run queries in parallel while in a transaction so there's that too. Which warrants this change.

Good job @andrewmurraydavid! Race conditions are hard to track down :+1: