forgejo/forgejo

Fork 619

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

Merged

Gusted merged 2 commits from gusted/forgejo-getengine-defaultcontext into forgejo

2025-11-17 14:42:58 +01:00

AGit

Gusted commented

2025-11-16 23:57:12 +01:00

Owner

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 to 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.

 // SetDefaultEngine sets the default engine for db.
 func SetDefaultEngine(ctx context.Context, eng Engine) {
 	masterEngine, err := GetMasterEngine(eng)
 	if err == nil {
 		x = masterEngine
 	}
 	DefaultContext = &Context{
 		Context: ctx,
 		e:       eng,
 	}
 }

And SetDefaultEngine is called from

SetDefaultEngine(ctx, eng)

Where eng is the engine group.

No test (no clue where to begin even). Tested in staging environment.

Test

Configure database replicas.
Start Forgejo.
Verify Forgejo loads.
Stop the database replicas.
Verify Forgejo shows 500 errors.

Release notes

Bug fixes
- PR: get new session from enginegroup instead of masterengine

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 to 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. https://codeberg.org/forgejo/forgejo/src/commit/20f8572b923c83258a53f1629782953000dd0e75/models/db/engine.go#L220-L231 And `SetDefaultEngine` is called from https://codeberg.org/forgejo/forgejo/src/commit/20f8572b923c83258a53f1629782953000dd0e75/models/db/engine.go#L212 Where `eng` is the engine group. No test (no clue where to begin even). Tested in staging environment. ## Test 1. Configure database replicas. 2. Start Forgejo. 3. Verify Forgejo loads. 4. Stop the database replicas. 5. Verify Forgejo shows 500 errors.  ## Release notes  - Bug fixes - [PR](https://codeberg.org/forgejo/forgejo/pulls/10140): get new session from enginegroup instead of masterengine

Gusted added 1 commit

2025-11-16 23:57:12 +01:00

fix: get new session from enginegroup instead of masterengine

testing / frontend-checks (pull_request) Successful in 50s

Details

testing / backend-checks (pull_request) Successful in 3m0s

Details

issue-labels / backporting (pull_request_target) Has been skipped

Details

issue-labels / cascade (pull_request_target) Has been skipped

Details

issue-labels / release-notes (pull_request_target) Has been skipped

Details

requirements / merge-conditions (pull_request) Successful in 3s

Details

testing / test-pgsql (pull_request) Failing after 2m55s

Details

testing / test-sqlite (pull_request) Failing after 3m2s

Details

testing / test-unit (pull_request) Successful in 6m36s

Details

testing / test-remote-cacher (redis) (pull_request) Successful in 2m14s

Details

testing / test-remote-cacher (valkey) (pull_request) Successful in 2m14s

Details

testing / test-remote-cacher (garnet) (pull_request) Successful in 2m13s

Details

testing / test-remote-cacher (redict) (pull_request) Successful in 2m13s

Details

testing / test-e2e (pull_request) Successful in 16m35s

Details

testing / test-mysql (pull_request) Successful in 22m8s

Details

testing / security-check (pull_request) Has been skipped

Details

8015b94dc8

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 to 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 explictly 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.

No test (no clue where to begin even). Tested in staging environment.

Gusted added the

backport/v13.0/forgejo

labels

2025-11-16 23:59:16 +01:00

Gusted commented

2025-11-17 00:01:13 +01:00

Author

Owner

@pat-s FYI

Gusted added 1 commit

2025-11-17 00:49:48 +01:00

chore: don't return enginegroup when no replica is set

testing / frontend-checks (pull_request) Successful in 54s

Details

testing / backend-checks (pull_request) Successful in 3m19s

Details

testing / test-unit (pull_request) Successful in 6m3s

Details

testing / test-remote-cacher (redis) (pull_request) Successful in 2m20s

Details

testing / test-remote-cacher (valkey) (pull_request) Successful in 2m19s

Details

testing / test-remote-cacher (garnet) (pull_request) Successful in 2m21s

Details

testing / test-remote-cacher (redict) (pull_request) Successful in 2m21s

Details

testing / test-e2e (pull_request) Failing after 19m44s

Details

testing / test-mysql (pull_request) Successful in 32m28s

Details

testing / test-sqlite (pull_request) Successful in 39m9s

Details

testing / test-pgsql (pull_request) Successful in 45m20s

Details

testing / security-check (pull_request) Successful in 1m55s

Details

issue-labels / cascade (pull_request_target) Has been skipped

Details

requirements / merge-conditions (pull_request) Successful in 1s

Details

issue-labels / release-notes (pull_request_target) Successful in 34s

Details

milestone / set (pull_request_target) Successful in 3s

Details

issue-labels / backporting (pull_request_target) Successful in 25s

Details

76fc2344f7

Don't always return a enginegroup, if there's no replica just return the
main engine.

Gusted requested review from mfenniak

2025-11-17 00:50:41 +01:00

mfenniak approved these changes

2025-11-17 02:46:02 +01:00

mfenniak left a comment

Member

In the scope of this change to Forgejo, everything looks good to me.

My other thoughts here are out-of-scope of this PR.

With respect to your testing... I feel this is obvious but I want to point it out anyway... "5. Verify Forgejo shows 500 errors." is great for proving that this change works but a terrible behaviour for an application accessing a database cluster. 🤣 I see that xorm doesn't do any kind of health checking on the replica set to maintain access to just the online replicas. You might want to be cautious as this will make the reliability of your system worse -- for example, if you're going from (1 primary) to (1 primary + 2 replicas), the application becomes quite broken at one database node outage. I'm just curious as I'm always interested in scaling -- is there a mechanism in mind to handle this? Or is it a future requirement?

In the scope of this change to Forgejo, everything looks good to me. My other thoughts here are out-of-scope of this PR. With respect to your testing... I feel this is obvious but I want to point it out anyway... "5. Verify Forgejo shows 500 errors." is great for proving that this change works but a terrible behaviour for an application accessing a database cluster. 🤣 I see that xorm doesn't do any kind of health checking on the replica set to maintain access to just the online replicas. You might want to be cautious as this will make the reliability of your system worse -- for example, if you're going from (1 primary) to (1 primary + 2 replicas), the application becomes quite broken at one database node outage. I'm just curious as I'm always interested in scaling -- is there a mechanism in mind to handle this? Or is it a future requirement?

Gusted commented

2025-11-17 02:54:45 +01:00

Author

Owner

@mfenniak wrote in #10140 (comment):

I see that xorm doesn't do any kind of health checking on the replica set to maintain access to just the online replicas. You might want to be cautious as this will make the reliability of your system worse -- for example, if you're going from (1 primary) to (1 primary + 2 replicas), the application becomes quite broken at one database node outage. I'm just curious as I'm always interested in scaling -- is there a mechanism in mind to handle this? Or is it a future requirement?

It's very likely I'll cook up another solution for Codeberg that doesn't involve asking XORM to do load-balancing (ProxySQL or HAProxy, but everything at database HA/scaling feels and looks enterprise), after seeing how this works in XORM (and getting it to work to verify this exact concern) we decided to not even test it on production.

@mfenniak wrote in https://codeberg.org/forgejo/forgejo/pulls/10140#issuecomment-8311652: > I see that xorm doesn't do any kind of health checking on the replica set to maintain access to just the online replicas. You might want to be cautious as this will make the reliability of your system worse -- for example, if you're going from (1 primary) to (1 primary + 2 replicas), the application becomes quite broken at one database node outage. I'm just curious as I'm always interested in scaling -- is there a mechanism in mind to handle this? Or is it a future requirement? It's very likely I'll cook up another solution for Codeberg that doesn't involve asking XORM to do load-balancing (ProxySQL or HAProxy, but everything at database HA/scaling feels and looks enterprise), after seeing how this works in XORM (and getting it to work to verify this exact concern) we decided to not even test it on production.

0ko added the

worth a release-note

label

2025-11-17 05:22:54 +01:00

forgejo-release-notes-assistant commented

2025-11-17 05:23:38 +01:00

Member

Where does that come from?

The following is a preview of the release notes for this pull request, as they will appear in the upcoming release. They are derived from the content of the `release-notes/10140.md` file, if it exists, or the title of the pull request. They were also added at the bottom of the description of this pull request for easier reference.

This message and the release notes originate from a call to the release-notes-assistant.

@@ -19,3 +19,11 @@
 3. Verify Forgejo loads.
 4. Stop the database replicas.
-5. Verify Forgejo shows 500 errors.
\ No newline at end of file
+5. Verify Forgejo shows 500 errors.
+
+<!--start release-notes-assistant-->
+
+## Release notes
+<!--URL:https://codeberg.org/forgejo/forgejo-->
+- Bug fixes
+  - [PR](https://codeberg.org/forgejo/forgejo/pulls/10140): <!--number 10140 --><!--line 0 --><!--description Z2V0IG5ldyBzZXNzaW9uIGZyb20gZW5naW5lZ3JvdXAgaW5zdGVhZCBvZiBtYXN0ZXJlbmdpbmU=-->get new session from enginegroup instead of masterengine<!--description-->
+<!--end release-notes-assistant-->

Release notes

Bug fixes
- PR: get new session from enginegroup instead of masterengine

<details> <summary>Where does that come from?</summary> The following is a preview of the release notes for this pull request, as they will appear in the upcoming release. They are derived from the content of the `release-notes/10140.md` file, if it exists, or the title of the pull request. They were also added at the bottom of the description of this pull request for easier reference. This message and the release notes originate from a call to the [release-notes-assistant](https://code.forgejo.org/forgejo/release-notes-assistant). ```diff @@ -19,3 +19,11 @@ 3. Verify Forgejo loads. 4. Stop the database replicas. -5. Verify Forgejo shows 500 errors. \ No newline at end of file +5. Verify Forgejo shows 500 errors. + + + +## Release notes + +- Bug fixes + - [PR](https://codeberg.org/forgejo/forgejo/pulls/10140): get new session from enginegroup instead of masterengine + ``` </details>  ## Release notes  - Bug fixes - [PR](https://codeberg.org/forgejo/forgejo/pulls/10140): get new session from enginegroup instead of masterengine

Gusted merged commit afbd05c398 into forgejo

2025-11-17 14:42:58 +01:00

Gusted referenced this pull request from a commit

2025-11-17 14:43:01 +01:00

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