fix: get new session from enginegroup instead of masterengine (#10140)

Within Codeberg we are looking into distributing the database queries, we tried forgejo/forgejo!7212 on several occasions but never got it to work.

After a long debugging session in a staging environment I was able to find two bugs that made it impossible for this feature to work: forgejo/docs!1587 which resulted in replica engines never being configured and used if you followed the documentation. The other bug is what this patch intends to fix. In order to do some database operation, you need the database engine - it will first look if one is set for the context (only useful for transactions) and otherwise create a new session of the engine from the master engine `x`. The problem is that `x` is explicitly set to be the master engine and not the engine group (that includes the replica engines) - Unless the code uses `DefaultContext`, which is almost nowhere used after some great refactoring in Gitea to use the passed context, it did not use the replica engines.

Get engine from the `DefaultContext` (which is set to the enginegroup) and create a new session from that.

20f8572b92/models/db/engine.go (L220-L231)

And `SetDefaultEngine` is called from 20f8572b92/models/db/engine.go (L212)

Where `eng` is the engine group.

## Test

1. Configure database replicas.
2. Start Forgejo.
3. Verify Forgejo loads.
4. Stop the database replicas.
5. Verify Forgejo shows 500 errors.

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/10140
Reviewed-by: Mathieu Fenniak <mfenniak@noreply.codeberg.org>
Co-authored-by: Gusted <postmaster@gusted.xyz>
Co-committed-by: Gusted <postmaster@gusted.xyz>

models/db/context.go

@@ -75,7 +75,7 @@ func GetEngine(ctx context.Context) Engine {
 	if e := getEngine(ctx); e != nil {
 		return e
 	}
-	return x.Context(ctx)
+	return DefaultContext.(Engined).Engine().Context(ctx)
 }
 
 // getEngine will get a db Engine from this context or return nil

models/db/engine.go

@@ -96,9 +96,15 @@ func init() {
 	}
 }
 
+type xormEngineInterface interface {
+	xorm.EngineInterface
+	SetDefaultContext(context.Context)
+	SetConnMaxIdleTime(time.Duration)
+}
+
 // newXORMEngineGroup creates an xorm.EngineGroup (with one master and one or more slaves).
 // It assumes you have separate master and slave DSNs defined via the settings package.
-func newXORMEngineGroup() (Engine, error) {
+func newXORMEngineGroup() (xormEngineInterface, error) {
 	// Retrieve master DSN from settings.
 	masterConnStr, err := setting.DBMasterConnStr()
 	if err != nil {
@@ -125,6 +131,9 @@ func newXORMEngineGroup() (Engine, error) {
 	if err != nil {
 		return nil, fmt.Errorf("failed to load slave DSNs: %w", err)
 	}
+	if len(slaveConnStrs) == 0 {
+		return masterEngine, nil
+	}
 
 	var slaveEngines []*xorm.Engine
 	// Iterate over all slave DSNs and create engines
@@ -147,16 +156,7 @@ func newXORMEngineGroup() (Engine, error) {
 	if err != nil {
 		return nil, fmt.Errorf("failed to create engine group: %w", err)
 	}
-	return engineGroupWrapper{group}, nil
-}
-
-type engineGroupWrapper struct {
-	*xorm.EngineGroup
-}
-
-func (w engineGroupWrapper) AddHook(hook contexts.Hook) bool {
-	w.EngineGroup.AddHook(hook)
-	return true
+	return group, nil
 }
 
 // SyncAllTables sync the schemas of all tables
@@ -174,46 +174,40 @@ func SyncAllTables() error {
 
 // InitEngine initializes the xorm EngineGroup and sets it as db.DefaultContext
 func InitEngine(ctx context.Context) error {
-	xormEngine, err := newXORMEngineGroup()
+	eng, err := newXORMEngineGroup()
 	if err != nil {
 		return fmt.Errorf("failed to connect to database: %w", err)
 	}
-	// Try to cast to the concrete type to access diagnostic methods
-	if eng, ok := xormEngine.(engineGroupWrapper); ok {
-		eng.SetMapper(names.GonicMapper{})
-		// WARNING: for serv command, MUST remove the output to os.Stdout,
-		// so use a log file instead of printing to stdout.
-		eng.SetLogger(NewXORMLogger(setting.Database.LogSQL))
-		eng.ShowSQL(setting.Database.LogSQL)
-		eng.SetMaxOpenConns(setting.Database.MaxOpenConns)
-		eng.SetMaxIdleConns(setting.Database.MaxIdleConns)
-		eng.SetConnMaxLifetime(setting.Database.ConnMaxLifetime)
-		eng.SetConnMaxIdleTime(setting.Database.ConnMaxIdleTime)
-		eng.SetDefaultContext(ctx)
+	eng.SetMapper(names.GonicMapper{})
+	// WARNING: for serv command, MUST remove the output to os.Stdout,
+	// so use a log file instead of printing to stdout.
+	eng.SetLogger(NewXORMLogger(setting.Database.LogSQL))
+	eng.ShowSQL(setting.Database.LogSQL)
+	eng.SetMaxOpenConns(setting.Database.MaxOpenConns)
+	eng.SetMaxIdleConns(setting.Database.MaxIdleConns)
+	eng.SetConnMaxLifetime(setting.Database.ConnMaxLifetime)
+	eng.SetConnMaxIdleTime(setting.Database.ConnMaxIdleTime)
+	eng.SetDefaultContext(ctx)
 
-		if setting.Database.SlowQueryThreshold > 0 {
-			eng.AddHook(&SlowQueryHook{
-				Threshold: setting.Database.SlowQueryThreshold,
-				Logger:    log.GetLogger("xorm"),
-			})
-		}
-
-		errorLogger := log.GetLogger("xorm")
-		if setting.IsInTesting {
-			errorLogger = log.GetLogger(log.DEFAULT)
-		}
-
-		eng.AddHook(&ErrorQueryHook{
-			Logger: errorLogger,
+	if setting.Database.SlowQueryThreshold > 0 {
+		eng.AddHook(&SlowQueryHook{
+			Threshold: setting.Database.SlowQueryThreshold,
+			Logger:    log.GetLogger("xorm"),
 		})
-
-		eng.AddHook(&TracingHook{})
-
-		SetDefaultEngine(ctx, eng)
-	} else {
-		// Fallback: if type assertion fails, set default engine without extended diagnostics
-		SetDefaultEngine(ctx, xormEngine)
 	}
+
+	errorLogger := log.GetLogger("xorm")
+	if setting.IsInTesting {
+		errorLogger = log.GetLogger(log.DEFAULT)
+	}
+
+	eng.AddHook(&ErrorQueryHook{
+		Logger: errorLogger,
+	})
+
+	eng.AddHook(&TracingHook{})
+
+	SetDefaultEngine(ctx, eng)
 	return nil
 }
 
@@ -368,8 +362,7 @@ func SetLogSQL(ctx context.Context, on bool) {
 
 	if sess, ok := ctxEngine.(*xorm.Session); ok {
 		sess.Engine().ShowSQL(on)
-	} else if wrapper, ok := ctxEngine.(engineGroupWrapper); ok {
-		// Handle engineGroupWrapper directly
+	} else if wrapper, ok := ctxEngine.(xormEngineInterface); ok {
 		wrapper.ShowSQL(on)
 	} else if masterEngine, err := GetMasterEngine(ctxEngine); err == nil {
 		masterEngine.ShowSQL(on)

modules/setting/database.go

@@ -14,8 +14,6 @@ import (
 	"strings"
 	"time"
 
-	"forgejo.org/modules/log"
-
 	"xorm.io/xorm"
 )
 
@@ -157,15 +155,6 @@ func DBSlaveConnStrs() ([]string, error) {
 			dsns = append(dsns, dsn)
 		}
 	}
-	// Fall back to master if no slave DSN was provided.
-	if len(dsns) == 0 {
-		master, err := DBMasterConnStr()
-		if err != nil {
-			return nil, err
-		}
-		log.Debug("Database: No dedicated replica host defined; falling back to primary DSN for replica connections")
-		dsns = append(dsns, master)
-	}
 	return dsns, nil
 }