~sircmpwn/sr.ht#301: GraphQL webhooks: evaluate race conditions for deletion events

~sircmpwn added beta 2 years ago

~adnano 2 years ago

This doesn't seem to be that big of a problem for meta.sr.ht. All the data that the user could possibly request is loaded during deletion, with the exception of the user field which uses the data loader. The only way I can think of for this to go wrong would be:

User deletes SSH key

Webhook scheduled

User deletes their account (not possible yet)

Webhook query is executed. The query requests the user field, which fails as the user does not exist.

This is a possibility, though it seems unlikely.

However, this will be a problem for git.sr.ht's new GraphQL-native webhooks. The Repository type has fields that will fail after the repository is deleted (e.g. log, path, etc). Here's an example query which can cause an error:
query {
  webhook {
    ... on RepositoryEvent {
      repository {
        log { results { id } }
      }
    }
  }
}
The log field resolver will fail as the repository has already been deleted.

In order to solve this, we have two options:

Execute the queries before deletion, then schedule the payloads to be sent.

Use a custom type for webhook payloads which only contains static data.

~sircmpwn 2 years ago

Execute the queries before deletion, then schedule the payloads to be sent.

This may be the better of the two solutions. There might also be a third, however: before we commit the resolver's SQL transaction, can we open a transaction for the worker task which has its isolation level appropriately tuned such that the deleted rows are still visible to it after the resolver commits? Will require some research/testing to see if this is possible.

~adnano 2 years ago

This may be the better of the two solutions. There might also be a third, however: before we commit the resolver's SQL transaction, can we open a transaction for the worker task which has its isolation level appropriately tuned such that the deleted rows are still visible to it after the resolver commits? Will require some research/testing to see if this is possible.

I considered this, but then I realized that when a repository is deleted, the contents are permanently removed with os.RemoveAll. Therefore, opening a transaction before deleting the repository won't help for the query above, which depends on the contents of the repository in the filesystem.

It seems like executing the queries before deletion might be our only option. Alternatively, we could say that fetching the log of a repository (or any other filesystem-dependent fields) upon deletion is not supported.

~sircmpwn 2 years ago

On Wed Jan 19, 2022 at 2:56 AM CET, ~adnano wrote:

It seems like executing the queries before deletion might be our only option. Alternatively, we could say that fetching the log of a repository (or any other filesystem-dependent fields) upon deletion is not supported.

That would be a bit too arbitrary. Let's execute queries before deletion, then. It's not perfect, but hey.

~adnano 2 years ago

Do we want to do this for all webhooks, or only for deletion events? Having only one codepath would be simpler.

~sircmpwn 2 years ago

We can do it for just one for now, but I think in general it makes sense for us to do it separately. Deletions are the least common kind of mutation and making everything slower for their sake is not great. We may also want to implement deletion by setting a "deleted" flag in the database, kicking off an async job to process webhooks and whatever else, then remove the row when all's said and done.