Files
forgejo/models/db/context_test.go
Mathieu Fenniak a9452d11d0 fix: possible cause of invalid issue counts; cache invalidation occurs before a active transaction is committed (#10130)
Although #9922 was deployed to Codeberg, it was reported on Matrix that a user observed a `-1` pull request count.

@Gusted checked and verified that the stats stored in redis appeared incorrect, and that no errors occurred on Codeberg that included the repo ID (eg. deadlocks, SQL queries).
```
127.0.0.1:6379> GET Repo:CountPulls:924266
"1"
127.0.0.1:6379> GET Repo:CountPullsClosed:924266
"2"
```

One possible cause is that when `UpdateRepoIssueNumbers` is invoked and invalidates the cache key for the repository, it is currently in a transaction; the next request for that cached count could be computed before the transaction is committed and the update is visible.  It's been verified that `UpdateRepoIssueNumbers` is called within a transaction in most interactions (I put a panic in it if `db.InTransaction(ctx)`, and most related tests failed).

This PR fixes that hole by performing the cache invalidation in an `AfterTx()` hook which is invoked after the transaction is committed to the database.

(Another possible cause is documented in #10127)

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/10130
Reviewed-by: Gusted <gusted@noreply.codeberg.org>
Co-authored-by: Mathieu Fenniak <mathieu@fenniak.net>
Co-committed-by: Mathieu Fenniak <mathieu@fenniak.net>
2025-11-17 01:07:29 +01:00

223 lines
5.4 KiB
Go

// Copyright 2022 The Gitea Authors. All rights reserved.
// SPDX-License-Identifier: MIT
package db_test
import (
"context"
"errors"
"fmt"
"testing"
"forgejo.org/models/db"
issues_model "forgejo.org/models/issues"
"forgejo.org/models/unittest"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)
func TestInTransaction(t *testing.T) {
require.NoError(t, unittest.PrepareTestDatabase())
assert.False(t, db.InTransaction(db.DefaultContext))
require.NoError(t, db.WithTx(db.DefaultContext, func(ctx context.Context) error {
assert.True(t, db.InTransaction(ctx))
return nil
}))
ctx, committer, err := db.TxContext(db.DefaultContext)
require.NoError(t, err)
defer committer.Close()
assert.True(t, db.InTransaction(ctx))
require.NoError(t, db.WithTx(ctx, func(ctx context.Context) error {
assert.True(t, db.InTransaction(ctx))
return nil
}))
}
func TestTxContext(t *testing.T) {
require.NoError(t, unittest.PrepareTestDatabase())
{ // create new transaction
ctx, committer, err := db.TxContext(db.DefaultContext)
require.NoError(t, err)
assert.True(t, db.InTransaction(ctx))
require.NoError(t, committer.Commit())
}
{ // reuse the transaction created by TxContext and commit it
ctx, committer, err := db.TxContext(db.DefaultContext)
engine := db.GetEngine(ctx)
require.NoError(t, err)
assert.True(t, db.InTransaction(ctx))
{
ctx, committer, err := db.TxContext(ctx)
require.NoError(t, err)
assert.True(t, db.InTransaction(ctx))
assert.Equal(t, engine, db.GetEngine(ctx))
require.NoError(t, committer.Commit())
}
require.NoError(t, committer.Commit())
}
{ // reuse the transaction created by TxContext and close it
ctx, committer, err := db.TxContext(db.DefaultContext)
engine := db.GetEngine(ctx)
require.NoError(t, err)
assert.True(t, db.InTransaction(ctx))
{
ctx, committer, err := db.TxContext(ctx)
require.NoError(t, err)
assert.True(t, db.InTransaction(ctx))
assert.Equal(t, engine, db.GetEngine(ctx))
require.NoError(t, committer.Close())
}
require.NoError(t, committer.Close())
}
{ // reuse the transaction created by WithTx
require.NoError(t, db.WithTx(db.DefaultContext, func(ctx context.Context) error {
assert.True(t, db.InTransaction(ctx))
{
ctx, committer, err := db.TxContext(ctx)
require.NoError(t, err)
assert.True(t, db.InTransaction(ctx))
require.NoError(t, committer.Commit())
}
return nil
}))
}
t.Run("Reuses parent context", func(t *testing.T) {
type unique struct{}
ctx := context.WithValue(db.DefaultContext, unique{}, "yes!")
assert.False(t, db.InTransaction(ctx))
require.NoError(t, db.WithTx(ctx, func(ctx context.Context) error {
assert.Equal(t, "yes!", ctx.Value(unique{}))
return nil
}))
})
}
func TestAfterTx(t *testing.T) {
tests := []struct {
executionMode string
rollback bool
}{
{
executionMode: "NoTx",
},
{
executionMode: "WithTx",
},
{
executionMode: "WithTxNested",
},
{
executionMode: "WithTx",
rollback: true,
},
{
executionMode: "WithTxNested",
rollback: true,
},
{
executionMode: "TxContext",
},
{
executionMode: "TxContextNested",
},
{
executionMode: "TxContext",
rollback: true,
},
{
executionMode: "TxContextNested",
rollback: true,
},
}
for _, tc := range tests {
t.Run(fmt.Sprintf("%s/%v", tc.executionMode, tc.rollback), func(t *testing.T) {
require.NoError(t, unittest.PrepareTestDatabase())
ctx := t.Context()
var err error
var countBefore, countAfter, hookCount int64
countBefore, err = db.GetEngine(ctx).Count(&issues_model.PullRequest{})
require.NoError(t, err)
sut := func(ctx context.Context) {
_, err = db.GetEngine(ctx).Insert(
&issues_model.PullRequest{IssueID: 2, BaseRepoID: 1, HeadRepoID: 1000})
require.NoError(t, err)
db.AfterTx(ctx, func() {
countAfter, err = db.GetEngine(ctx).Count(&issues_model.PullRequest{})
require.NoError(t, err)
assert.False(t, db.InTransaction(ctx))
hookCount++
})
}
switch tc.executionMode {
case "NoTx":
sut(ctx)
case "WithTx":
db.WithTx(ctx, func(ctx context.Context) error {
sut(ctx)
if tc.rollback {
return errors.New("rollback")
}
return nil
})
case "WithTxNested":
db.WithTx(ctx, func(ctx context.Context) error {
return db.WithTx(ctx, func(ctx context.Context) error {
sut(ctx)
if tc.rollback {
return errors.New("rollback")
}
return nil
})
})
case "TxContext":
txCtx, committer, err := db.TxContext(ctx)
require.NoError(t, err)
sut(txCtx)
if !tc.rollback {
err = committer.Commit()
require.NoError(t, err)
}
committer.Close()
case "TxContextNested":
txCtx1, committer1, err := db.TxContext(ctx)
require.NoError(t, err)
txCtx2, committer2, err := db.TxContext(txCtx1)
require.NoError(t, err)
sut(txCtx2)
err = committer2.Commit()
require.NoError(t, err)
committer2.Close()
if !tc.rollback {
err = committer1.Commit()
require.NoError(t, err)
}
committer1.Close()
default:
t.Fatalf("unexpected execution mode: %q", tc.executionMode)
}
if tc.rollback {
assert.EqualValues(t, 0, hookCount)
assert.EqualValues(t, 0, countAfter)
} else {
assert.EqualValues(t, 1, hookCount)
assert.Equal(t, countBefore+1, countAfter)
}
})
}
}