Deletes are not getting committed on CockroachDB

viragtripathi commented 3 months ago

Preflight checklist

[X] I could not find a solution in the existing issues, docs, nor discussions.
[X] I agree to follow this project's Code of Conduct.
[X] I have read and am following this repository's Contribution Guidelines.
[ ] I have joined the Ory Community Slack.
[ ] I am signed up to the Ory Security Patch Newsletter.

Ory Network Project

No response

Describe the bug

We observed that delete queries are not getting executed within a transaction boundary within CockroachDB. Here is an example query

DELETE FROM
  hydra_oauth2_access AS hydra_oauth2_access
WHERE
  nid = '39bba160-9c5d-4bd1-9f80-170c4f7c4a06':::UUID AND request_id = 'bb7c2273-eaba-4e17-9dab-bec895e93b55':::STRING

After further investigation, we suspect that delete queries created by this function is not adhering to transaction boundary, hence not getting committed. Please see, https://github.com/ory/hydra/blob/master/persistence/sql/persister_oauth2.go#L381

code snippet from above url

func (p *Persister) DeleteAccessTokenSession(ctx context.Context, signature string) (err error) {
    ctx, span := p.r.Tracer(ctx).Tracer().Start(ctx, "persistence.sql.DeleteAccessTokenSession")
    defer otelx.End(span, &err)

    err = sqlcon.HandleError(
        p.QueryWithNetwork(ctx).
            Where("signature = ?", SignatureHash(signature)).
            Delete(&OAuth2RequestSQL{Table: sqlTableAccess}))
    if errors.Is(err, sqlcon.ErrNoRows) {
        // Backwards compatibility: we previously did not always hash the
        // signature before inserting. In case there are still very old (but
        // valid) access tokens in the database, this should get them.
        err = sqlcon.HandleError(
            p.QueryWithNetwork(ctx).
                Where("signature = ?", signature).
                Delete(&OAuth2RequestSQL{Table: sqlTableAccess}))
        if errors.Is(err, sqlcon.ErrNoRows) {
            return errorsx.WithStack(fosite.ErrNotFound)
        }
    }
    if errors.Is(err, sqlcon.ErrConcurrentUpdate) {
        return errors.Wrap(fosite.ErrSerializationFailure, err.Error())
    }
    return err
}

We see that gobuffalo/pop orm is being used in your code for the sql queries but it's not clear if pop is auto committing or you have an explicit transaction utility to handle the commits.

Due to this behavior, we see a lot of transaction contention in CockroachDB.

Reproducing the bug

Use CockroachDB dedicated
Run > 55K tps

Relevant log output

No response

Relevant configuration

No response

Version

v2.2.0 and v2.2.0-rc.2

On which operating system are you observing this issue?

Linux

In which environment are you deploying?

Kubernetes

Additional Context

No response

viragtripathi commented 3 months ago

@david7joy @nollenr

aeneasr commented 2 months ago

As far as we know, statements which are without a transcation are implicitly executed in a transaction. It is possible that we're not setting an explicit transaction here.

One question I have is though, transaction contention probably happens when we're accessing a row for reading while at the same time modifying it (update or delete). Is this true for CRDB?

ory / hydra