forgejo/forgejo
forgejo/forgejo
120
4.9k
Fork 842
Code Issues 1.3k Pull requests 152 Projects Releases 106 Packages 1 Activity Actions 10

fix: validate and buffer downloaded assets before S3 upload during migrations #8865

Closed
pat-s wants to merge 3 commits from pat-s/forgejo:fix/migrate-buffer-s3 into forgejo
pull from: pat-s/forgejo:fix/migrate-buffer-s3
merge into: forgejo:forgejo
Conversation 18 Commits 3 Files changed 3 +156 -2
pat-s commented 2025-08-12 00:57:24 +02:00
Member
Copy link

This fixes an issue when migrating a repo (with release assets) into a FJ instance with an S3 backend for asset storage.
It ensures that the download fully completes and is validated before it is pushed to S3, preventing discrepancies between the expected content length and the actual body.

Without this fix, the following error occurs during a migration:

services/task/task.go:54:handler() [E] Run task failed: Put "https://<s3 url>/attachments/6/9/69a38d07-9e44-4697-b2f6-0b5a327e4b6c": http: ContentLength=1218036 with Body length 918

Before: GitHub Download → S3 Upload (while streaming)
After: GitHub → [Download Complete] → [Buffer & Validate] → S3 Storage

I verified that this patch prevents the error reported above.

(should likely be backported)

Checklist

The contributor guide contains information that will be helpful to first time contributors. There also are a few conditions for merging Pull Requests in Forgejo repositories. You are also welcome to join the Forgejo development chatroom.

Tests

  • I added test coverage for Go changes...
    • in their respective *_test.go for unit tests.
    • in the tests/integration directory if it involves interactions with a live Forgejo server.
  • I added test coverage for JavaScript changes...

Documentation

  • I created a pull request to the documentation to explain to Forgejo users how to use this change.
  • I did not document these changes and I do not expect someone else to do it.

Release notes

  • I do not want this change to show in the release notes.
  • I want the title to show in the release notes with a link to this pull request.
  • I want the content of the release-notes/<pull request number>.md to be be used for the release notes instead of the title.
This fixes an issue when migrating a repo (with release assets) into a FJ instance with an S3 backend for asset storage. It ensures that the download fully completes and is validated before it is pushed to S3, preventing discrepancies between the expected content length and the actual body. Without this fix, the following error occurs during a migration: ```sh services/task/task.go:54:handler() [E] Run task failed: Put "https://<s3 url>/attachments/6/9/69a38d07-9e44-4697-b2f6-0b5a327e4b6c": http: ContentLength=1218036 with Body length 918 ``` Before: GitHub Download → S3 Upload (while streaming) After: GitHub → [Download Complete] → [Buffer & Validate] → S3 Storage I verified that this patch prevents the error reported above. (should likely be backported) ## Checklist The [contributor guide](https://forgejo.org/docs/next/contributor/) contains information that will be helpful to first time contributors. There also are a few [conditions for merging Pull Requests in Forgejo repositories](https://codeberg.org/forgejo/governance/src/branch/main/PullRequestsAgreement.md). You are also welcome to join the [Forgejo development chatroom](https://matrix.to/#/#forgejo-development:matrix.org). ### Tests - I added test coverage for Go changes... - [x] in their respective `*_test.go` for unit tests. - [ ] in the `tests/integration` directory if it involves interactions with a live Forgejo server. - I added test coverage for JavaScript changes... - [ ] in `web_src/js/*.test.js` if it can be unit tested. - [ ] in `tests/e2e/*.test.e2e.js` if it requires interactions with a live Forgejo server (see also the [developer guide for JavaScript testing](https://codeberg.org/forgejo/forgejo/src/branch/forgejo/tests/e2e/README.md#end-to-end-tests)). ### Documentation - [ ] I created a pull request [to the documentation](https://codeberg.org/forgejo/docs) to explain to Forgejo users how to use this change. - [x] I did not document these changes and I do not expect someone else to do it. ### Release notes - [ ] I do not want this change to show in the release notes. - [x] I want the title to show in the release notes with a link to this pull request. - [ ] I want the content of the `release-notes/<pull request number>.md` to be be used for the release notes instead of the title.
add
All checks were successful
testing / frontend-checks (pull_request) Successful in 50s
Details
requirements / merge-conditions (pull_request) Successful in 2s
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
testing / backend-checks (pull_request) Successful in 3m1s
Details
testing / test-unit (pull_request) Successful in 5m54s
Details
testing / test-remote-cacher (redis) (pull_request) Successful in 1m57s
Details
testing / test-remote-cacher (valkey) (pull_request) Successful in 1m57s
Details
testing / test-remote-cacher (garnet) (pull_request) Successful in 1m59s
Details
testing / test-remote-cacher (redict) (pull_request) Successful in 2m0s
Details
testing / test-e2e (pull_request) Successful in 15m39s
Details
testing / test-mysql (pull_request) Successful in 17m28s
Details
testing / test-sqlite (pull_request) Successful in 22m35s
Details
testing / test-pgsql (pull_request) Successful in 25m17s
Details
testing / security-check (pull_request) Successful in 42s
Details
7b8614b1d9
earl-warren commented 2025-08-12 15:03:14 +02:00
Contributor
Copy link

Could you explain the root cause of this fix? I'm not seeing it. I do get that the asset is read in memory before being sent, but I don't get why it would fail if it is not? Why is there a size difference between the advertised content length and the size of the body?

Could you explain the root cause of this fix? I'm not seeing it. I do get that the asset is read in memory before being sent, but I don't get why it would fail if it is not? Why is there a size difference between the advertised content length and the size of the body?
earl-warren commented 2025-08-12 15:05:34 +02:00
Contributor
Copy link

The downside of this fix is that large assets will significantly more memory when migrating. I'm not saying it is a blocker in case this is the only way to fix the bug.

The downside of this fix is that large assets will significantly more memory when migrating. I'm not saying it is a blocker in case this is the only way to fix the bug.
pat-s commented 2025-08-12 15:10:37 +02:00
Author
Member
Copy link

I don't know why that is, I only see that there is a difference.

The process flow I posted in OP is more or less just a guess based on the result (i.e. a premature write request before the actual download is fully complete and/or buffered, causing body length != content length for the subsequent upload request).

The issue should be reproducible with any instance that uses S3 for asset storage and also unlikely to be linked to the size of release assets. At first I thought it might be related to reverse proxy settings, but I was able to out rule it after some tests. Then I checked for the above and it turned out to be the culprit.

I don't know *why* that is, I only see *that* there is a difference. The process flow I posted in OP is more or less just a guess based on the result (i.e. a premature write request before the actual download is fully complete and/or buffered, causing body length != content length for the subsequent upload request). The issue should be reproducible with any instance that uses S3 for asset storage and also unlikely to be linked to the size of release assets. At first I thought it might be related to reverse proxy settings, but I was able to out rule it after some tests. Then I checked for the above and it turned out to be the culprit.
pat-s commented 2025-08-12 15:13:53 +02:00
Author
Member
Copy link

The downside of this fix is that large assets will significantly more memory when migrating. I'm not saying it is a blocker in case this is the only way to fix the bug.

And it might be slower overall since there is more waiting time for the new process. But I don't mind if the alternative is that the migration would error out.

Happy to take a different approach if you have one, but I generally also think this fine overall.
You won't be able to reproduce it on Codeberg as S3 isn't used for the asset storage, but if you have another instance with S3, it might be worth giving it a try.

(I fell over this yesterday attempting to migrate one of my repos from GH).

> The downside of this fix is that large assets will significantly more memory when migrating. I'm not saying it is a blocker in case this is the only way to fix the bug. And it might be slower overall since there is more waiting time for the new process. But I don't mind if the alternative is that the migration would error out. Happy to take a different approach if you have one, but I generally also think this fine overall. You won't be able to reproduce it on Codeberg as S3 isn't used for the asset storage, but if you have another instance with S3, it might be worth giving it a try. (I fell over this yesterday attempting to migrate [one of my repos from GH](https://github.com/pat-s/alfred-gitea)).
earl-warren commented 2025-08-12 16:06:34 +02:00
Contributor
Copy link

I'm surprised that was never reported but it is good you can reproduce it. Can you add a reproducer in the integration tests? t.Skip("Test skipped for non-Minio-storage.") is one place you can check see how to guard a Minio specific test.

I'm surprised that was never reported but it is good you can reproduce it. Can you add a reproducer in the integration tests? `t.Skip("Test skipped for non-Minio-storage.")` is one place you can check see how to guard a Minio specific test.
pat-s commented 2025-08-12 16:59:45 +02:00
Author
Member
Copy link

I'm surprised that was never reported

Me too but I guess most instances don't use S3 for asset storage (?). Maybe there's also another player in all of this which I am not seeing yet. But given that the patch here makes it work/fail reliably, it looks to me as if this is the main culprit.
I'd appreciate if one with an s3 asset backend could also double-verify this issue.

(Maybe all of this is also somehow related to the S3/minio closer debate a few weeks ago.)

> I'm surprised that was never reported Me too but I guess most instances don't use S3 for asset storage (?). Maybe there's also another player in all of this which I am not seeing yet. But given that the patch here makes it work/fail reliably, it looks to me as if this is the main culprit. I'd appreciate if one with an s3 asset backend could also double-verify this issue. (Maybe all of this is also somehow related to the S3/minio closer debate a few weeks ago.)
earl-warren commented 2025-08-12 18:05:08 +02:00
Contributor
Copy link

@pat-s wrote in #8865 (comment):

(Maybe all of this is also somehow related to the S3/minio closer debate a few weeks ago.)

That's precisely why it would be most valuable to have a reproducer.

@pat-s wrote in #8865 (comment):

I'd appreciate if one with an s3 asset backend could also double-verify this issue.

Let's wait a few days for someone to manifest themselves.

@pat-s wrote in https://codeberg.org/forgejo/forgejo/pulls/8865#issuecomment-6270160: > (Maybe all of this is also somehow related to the S3/minio closer debate a few weeks ago.) That's precisely why it would be most valuable to have a reproducer. @pat-s wrote in https://codeberg.org/forgejo/forgejo/pulls/8865#issuecomment-6270160: > I'd appreciate if one with an s3 asset backend could also double-verify this issue. Let's wait a few days for someone to manifest themselves.
add test
All checks were successful
requirements / merge-conditions (pull_request) Successful in 3s
Details
issue-labels / release-notes (pull_request_target) Has been skipped
Details
testing / frontend-checks (pull_request) Successful in 1m54s
Details
testing / backend-checks (pull_request) Successful in 4m44s
Details
testing / test-unit (pull_request) Successful in 7m36s
Details
testing / test-remote-cacher (redis) (pull_request) Successful in 1m56s
Details
testing / test-remote-cacher (valkey) (pull_request) Successful in 2m6s
Details
testing / test-remote-cacher (garnet) (pull_request) Successful in 2m4s
Details
testing / test-remote-cacher (redict) (pull_request) Successful in 2m5s
Details
testing / test-e2e (pull_request) Successful in 20m57s
Details
testing / test-mysql (pull_request) Successful in 22m42s
Details
testing / test-sqlite (pull_request) Successful in 27m26s
Details
testing / test-pgsql (pull_request) Successful in 30m45s
Details
testing / security-check (pull_request) Successful in 46s
Details
8f3170c0d0
earl-warren reviewed 2025-08-14 11:38:29 +02:00
tests/integration/api_repo_migrate_releases_s3_test.go Outdated
@ -0,0 +23,4 @@
return true
}
func TestValidateAndBufferAsset(t *testing.T) {
earl-warren commented 2025-08-14 11:38:29 +02:00
Contributor
Copy link

Does it fail without the change?

Does it fail without the change?
pat-s commented 2025-08-14 14:46:01 +02:00
Author
Member
Copy link

I hope the new test can serve as a proof.

I hope the new test can serve as a proof.
add test showcasing the error for unbuffered s3 uploads
All checks were successful
requirements / merge-conditions (pull_request) Successful in 1s
Details
issue-labels / release-notes (pull_request_target) Has been skipped
Details
testing / frontend-checks (pull_request) Successful in 1m5s
Details
testing / backend-checks (pull_request) Successful in 2m56s
Details
testing / test-unit (pull_request) Successful in 6m49s
Details
testing / test-remote-cacher (redis) (pull_request) Successful in 2m25s
Details
testing / test-remote-cacher (valkey) (pull_request) Successful in 2m23s
Details
testing / test-remote-cacher (garnet) (pull_request) Successful in 2m24s
Details
testing / test-remote-cacher (redict) (pull_request) Successful in 2m26s
Details
testing / test-e2e (pull_request) Successful in 21m40s
Details
testing / test-mysql (pull_request) Successful in 23m25s
Details
testing / test-sqlite (pull_request) Successful in 28m20s
Details
testing / test-pgsql (pull_request) Successful in 31m18s
Details
testing / security-check (pull_request) Successful in 50s
Details
issue-labels / backporting (pull_request_target) Has been skipped
Details
milestone / set (pull_request_target) Has been skipped
Details
5ceee02792
earl-warren reviewed 2025-08-14 15:51:34 +02:00
tests/integration/api_repo_migrate_releases_s3_test.go
@ -0,0 +34,4 @@
return n, nil
}
func isS3Storage() bool {
earl-warren commented 2025-08-14 15:51:34 +02:00
Contributor
Copy link

it returns true even if not S3

it returns true even if not S3
earl-warren requested changes 2025-08-14 16:16:20 +02:00
earl-warren left a comment
Copy link

I ran the tests locally with the change reverted and it passes. Marking a request changes until there is either a reproducer or report from other users to pursue this further.

$ TEST_PGSQL_HOST=localhost:5432 TEST_PGSQL_DBNAME=test TEST_PGSQL_USERNAME=postgres TEST_PGSQL_PASSWORD=postgres TEST_S3_HOST=localhost:9000 make 'test-pgsql#TestValidateAndBufferAsset'
/home/earl-warren/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.24.6.linux-amd64/bin/go test  -c forgejo.org/tests/integration -o integrations.pgsql.test
sed -e 's|{{TEST_PGSQL_HOST}}|localhost:5432|g' \
	-e 's|{{TEST_PGSQL_DBNAME}}|test|g' \
	-e 's|{{TEST_PGSQL_USERNAME}}|postgres|g' \
	-e 's|{{TEST_PGSQL_PASSWORD}}|postgres|g' \
	-e 's|{{TEST_PGSQL_SCHEMA}}|gtestschema|g' \
	-e 's|{{REPO_TEST_DIR}}||g' \
	-e 's|{{TEST_LOGGER}}|test,file|g' \
	-e 's|{{TEST_TYPE}}|integration|g' \
	-e 's|{{TEST_STORAGE_TYPE}}|minio|g' \
	-e 's|{{TEST_S3_HOST}}|localhost:9000|g' \
		tests/pgsql.ini.tmpl > tests/pgsql.ini
GITEA_ROOT="/home/earl-warren/software/forgejo" GITEA_CONF=tests/pgsql.ini  ./integrations.pgsql.test  -test.run TestValidateAndBufferAsset
2025/08/14 16:09:18 ...s/storage/storage.go:157:initAttachments() [I] Initialising Attachment storage with type: minio
2025/08/14 16:09:18 ...les/storage/minio.go:97:NewMinioStorage() [I] Creating Minio storage at localhost:9000:gitea with base path attachments/
2025/08/14 16:09:18 ...s/storage/storage.go:147:initAvatars() [I] Initialising Avatar storage with type: minio
2025/08/14 16:09:18 ...les/storage/minio.go:97:NewMinioStorage() [I] Creating Minio storage at localhost:9000:gitea with base path avatars/
2025/08/14 16:09:18 ...s/storage/storage.go:173:initRepoAvatars() [I] Initialising Repository Avatar storage with type: minio
2025/08/14 16:09:18 ...les/storage/minio.go:97:NewMinioStorage() [I] Creating Minio storage at localhost:9000:gitea with base path repo-avatars/
2025/08/14 16:09:18 ...s/storage/storage.go:167:initLFS() [I] Initialising LFS storage with type: minio
2025/08/14 16:09:18 ...les/storage/minio.go:97:NewMinioStorage() [I] Creating Minio storage at localhost:9000:gitea with base path lfs/
2025/08/14 16:09:18 ...s/storage/storage.go:179:initRepoArchives() [I] Initialising Repository Archive storage with type: minio
2025/08/14 16:09:18 ...les/storage/minio.go:97:NewMinioStorage() [I] Creating Minio storage at localhost:9000:gitea with base path repo-archive/
2025/08/14 16:09:18 ...s/storage/storage.go:189:initPackages() [I] Initialising Packages storage with type: minio
2025/08/14 16:09:18 ...les/storage/minio.go:97:NewMinioStorage() [I] Creating Minio storage at localhost:9000:gitea with base path packages/
2025/08/14 16:09:18 ...s/storage/storage.go:200:initActions() [I] Initialising Actions storage with type: minio
2025/08/14 16:09:18 ...les/storage/minio.go:97:NewMinioStorage() [I] Creating Minio storage at localhost:9000:gitea with base path actions_log/
2025/08/14 16:09:18 ...s/storage/storage.go:204:initActions() [I] Initialising ActionsArtifacts storage with type: minio
2025/08/14 16:09:18 ...les/storage/minio.go:97:NewMinioStorage() [I] Creating Minio storage at localhost:9000:gitea with base path actions_artifacts/
2025/08/14 16:09:18 modules/git/git.go:128:checkInit() [W] git module has been initialized already, duplicate init may work but it's better to fix it
2025/08/14 16:09:18 routers/init.go:115:InitWebInstalled() [I] Git version: 2.43.0, Wire Protocol Version 2 Enabled (home: /home/earl-warren/software/forgejo/tests/integration/gitea-integration-pgsql/data/home)
??? [TestLogger] 2025/08/14 16:09:18 ...s/storage/storage.go:157:initAttachments() [I] Initialising Attachment storage with type: minio
??? [TestLogger] 2025/08/14 16:09:18 ...les/storage/minio.go:97:NewMinioStorage() [I] Creating Minio storage at localhost:9000:gitea with base path attachments/
??? [TestLogger] 2025/08/14 16:09:18 ...s/storage/storage.go:147:initAvatars() [I] Initialising Avatar storage with type: minio
??? [TestLogger] 2025/08/14 16:09:18 ...les/storage/minio.go:97:NewMinioStorage() [I] Creating Minio storage at localhost:9000:gitea with base path avatars/
??? [TestLogger] 2025/08/14 16:09:18 ...s/storage/storage.go:173:initRepoAvatars() [I] Initialising Repository Avatar storage with type: minio
??? [TestLogger] 2025/08/14 16:09:18 ...les/storage/minio.go:97:NewMinioStorage() [I] Creating Minio storage at localhost:9000:gitea with base path repo-avatars/
??? [TestLogger] 2025/08/14 16:09:18 ...s/storage/storage.go:167:initLFS() [I] Initialising LFS storage with type: minio
??? [TestLogger] 2025/08/14 16:09:18 ...les/storage/minio.go:97:NewMinioStorage() [I] Creating Minio storage at localhost:9000:gitea with base path lfs/
??? [TestLogger] 2025/08/14 16:09:18 ...s/storage/storage.go:179:initRepoArchives() [I] Initialising Repository Archive storage with type: minio
??? [TestLogger] 2025/08/14 16:09:18 ...les/storage/minio.go:97:NewMinioStorage() [I] Creating Minio storage at localhost:9000:gitea with base path repo-archive/
??? [TestLogger] 2025/08/14 16:09:18 ...s/storage/storage.go:189:initPackages() [I] Initialising Packages storage with type: minio
??? [TestLogger] 2025/08/14 16:09:18 ...les/storage/minio.go:97:NewMinioStorage() [I] Creating Minio storage at localhost:9000:gitea with base path packages/
??? [TestLogger] 2025/08/14 16:09:18 ...s/storage/storage.go:200:initActions() [I] Initialising Actions storage with type: minio
??? [TestLogger] 2025/08/14 16:09:18 ...les/storage/minio.go:97:NewMinioStorage() [I] Creating Minio storage at localhost:9000:gitea with base path actions_log/
??? [TestLogger] 2025/08/14 16:09:18 ...s/storage/storage.go:204:initActions() [I] Initialising ActionsArtifacts storage with type: minio
??? [TestLogger] 2025/08/14 16:09:18 ...les/storage/minio.go:97:NewMinioStorage() [I] Creating Minio storage at localhost:9000:gitea with base path actions_artifacts/
??? [TestLogger] 2025/08/14 16:09:18 routers/common/db.go:23:InitDBEngine() [I] Beginning ORM engine initialization.
??? [TestLogger] 2025/08/14 16:09:18 routers/common/db.go:30:InitDBEngine() [I] ORM engine initialization attempt #1/10...
??? [TestLogger] 2025/08/14 16:09:20 routers/init.go:141:InitWebInstalled() [I] ORM engine initialization successful!
??? [TestLogger] 2025/08/14 16:09:20 ...er/issues/indexer.go:77:func1() [I] PID 211550: Initializing Issue Indexer: bleve
??? [TestLogger] 2025/08/14 16:09:20 ...exer/code/indexer.go:156:func3() [I] PID: 211550 Initializing Repository Indexer at: /home/earl-warren/software/forgejo/tests/integration/gitea-integration-pgsql/indexers/repos.bleve
??? [TestLogger] 2025/08/14 16:09:20 ...xer/stats/indexer.go:41:populateRepoIndexer() [I] Populating the repo stats indexer with existing repositories
??? [TestLogger] 2025/08/14 16:09:20 ...er/issues/indexer.go:154:func2() [I] Issue Indexer Initialization took 4.004266ms
??? [TestLogger] 2025/08/14 16:09:20 ...exer/code/indexer.go:231:func4() [I] Repository Indexer Initialization took 4.212531ms
??? [TestLogger] 2025/08/14 16:09:20 ...exer/code/indexer.go:257:populateRepoIndexer() [I] Populating the repo indexer with existing repositories
??? [TestLogger] 2025/08/14 16:09:20 routers/init.go:84:syncAppConfForGit() [I] AppPath changed from '' to '/home/earl-warren/software/forgejo/gitea'
??? [TestLogger] 2025/08/14 16:09:20 routers/init.go:89:syncAppConfForGit() [I] CustomConf changed from '' to '/home/earl-warren/software/forgejo/tests/pgsql.ini'
??? [TestLogger] 2025/08/14 16:09:20 routers/init.go:95:syncAppConfForGit() [I] re-sync repository hooks ...
??? [TestLogger] 2025/08/14 16:09:20 routers/init.go:98:syncAppConfForGit() [I] re-write ssh public keys ...
??? [TestLogger] 2025/08/14 16:09:20 modules/ssh/ssh.go:332:Listen() [I] Adding SSH host key: /home/earl-warren/software/forgejo/tests/integration/gitea-integration-pgsql/data/ssh/gitea.rsa
??? [TestLogger] 2025/08/14 16:09:20 modules/ssh/init.go:26:Init() [I] SSH server started on localhost:2202. Cipher list ([chacha20-poly1305@openssh.com aes128-ctr aes192-ctr aes256-ctr aes128-gcm@openssh.com aes256-gcm@openssh.com]), key exchange algorithms ([mlkem768x25519-sha256 curve25519-sha256 ecdh-sha2-nistp256 ecdh-sha2-nistp384 ecdh-sha2-nistp521 diffie-hellman-group14-sha256 diffie-hellman-group14-sha1]), MACs ([hmac-sha2-256-etm@openssh.com hmac-sha2-256 hmac-sha1])
??? [TestLogger] 2025/08/14 16:09:20 ...s/graceful/server.go:50:NewServer() [I] Starting new SSH server: tcp:localhost:2202 on PID: 211550
??? [TestLogger] 2025/08/14 16:09:20 ...er/cleanup_sha256.go:27:CleanupSHA256() [I] Start to cleanup dangling images with a sha256:* version
??? [TestLogger] 2025/08/14 16:09:20 ...er/cleanup_sha256.go:106:cleanupSHA256() [I] Nothing to cleanup
??? [TestLogger] 2025/08/14 16:09:20 ...er/cleanup_sha256.go:29:CleanupSHA256() [I] Finished to cleanup dangling images with a sha256:* version
=== TestValidateAndBufferAsset (tests/integration/api_repo_migrate_releases_s3_test.go:51)
PASS
I ran the tests locally with the change reverted and it passes. Marking a `request changes` until there is either a reproducer or report from other users to pursue this further. ```sh $ TEST_PGSQL_HOST=localhost:5432 TEST_PGSQL_DBNAME=test TEST_PGSQL_USERNAME=postgres TEST_PGSQL_PASSWORD=postgres TEST_S3_HOST=localhost:9000 make 'test-pgsql#TestValidateAndBufferAsset' /home/earl-warren/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.24.6.linux-amd64/bin/go test -c forgejo.org/tests/integration -o integrations.pgsql.test sed -e 's|{{TEST_PGSQL_HOST}}|localhost:5432|g' \ -e 's|{{TEST_PGSQL_DBNAME}}|test|g' \ -e 's|{{TEST_PGSQL_USERNAME}}|postgres|g' \ -e 's|{{TEST_PGSQL_PASSWORD}}|postgres|g' \ -e 's|{{TEST_PGSQL_SCHEMA}}|gtestschema|g' \ -e 's|{{REPO_TEST_DIR}}||g' \ -e 's|{{TEST_LOGGER}}|test,file|g' \ -e 's|{{TEST_TYPE}}|integration|g' \ -e 's|{{TEST_STORAGE_TYPE}}|minio|g' \ -e 's|{{TEST_S3_HOST}}|localhost:9000|g' \ tests/pgsql.ini.tmpl > tests/pgsql.ini GITEA_ROOT="/home/earl-warren/software/forgejo" GITEA_CONF=tests/pgsql.ini ./integrations.pgsql.test -test.run TestValidateAndBufferAsset 2025/08/14 16:09:18 ...s/storage/storage.go:157:initAttachments() [I] Initialising Attachment storage with type: minio 2025/08/14 16:09:18 ...les/storage/minio.go:97:NewMinioStorage() [I] Creating Minio storage at localhost:9000:gitea with base path attachments/ 2025/08/14 16:09:18 ...s/storage/storage.go:147:initAvatars() [I] Initialising Avatar storage with type: minio 2025/08/14 16:09:18 ...les/storage/minio.go:97:NewMinioStorage() [I] Creating Minio storage at localhost:9000:gitea with base path avatars/ 2025/08/14 16:09:18 ...s/storage/storage.go:173:initRepoAvatars() [I] Initialising Repository Avatar storage with type: minio 2025/08/14 16:09:18 ...les/storage/minio.go:97:NewMinioStorage() [I] Creating Minio storage at localhost:9000:gitea with base path repo-avatars/ 2025/08/14 16:09:18 ...s/storage/storage.go:167:initLFS() [I] Initialising LFS storage with type: minio 2025/08/14 16:09:18 ...les/storage/minio.go:97:NewMinioStorage() [I] Creating Minio storage at localhost:9000:gitea with base path lfs/ 2025/08/14 16:09:18 ...s/storage/storage.go:179:initRepoArchives() [I] Initialising Repository Archive storage with type: minio 2025/08/14 16:09:18 ...les/storage/minio.go:97:NewMinioStorage() [I] Creating Minio storage at localhost:9000:gitea with base path repo-archive/ 2025/08/14 16:09:18 ...s/storage/storage.go:189:initPackages() [I] Initialising Packages storage with type: minio 2025/08/14 16:09:18 ...les/storage/minio.go:97:NewMinioStorage() [I] Creating Minio storage at localhost:9000:gitea with base path packages/ 2025/08/14 16:09:18 ...s/storage/storage.go:200:initActions() [I] Initialising Actions storage with type: minio 2025/08/14 16:09:18 ...les/storage/minio.go:97:NewMinioStorage() [I] Creating Minio storage at localhost:9000:gitea with base path actions_log/ 2025/08/14 16:09:18 ...s/storage/storage.go:204:initActions() [I] Initialising ActionsArtifacts storage with type: minio 2025/08/14 16:09:18 ...les/storage/minio.go:97:NewMinioStorage() [I] Creating Minio storage at localhost:9000:gitea with base path actions_artifacts/ 2025/08/14 16:09:18 modules/git/git.go:128:checkInit() [W] git module has been initialized already, duplicate init may work but it's better to fix it 2025/08/14 16:09:18 routers/init.go:115:InitWebInstalled() [I] Git version: 2.43.0, Wire Protocol Version 2 Enabled (home: /home/earl-warren/software/forgejo/tests/integration/gitea-integration-pgsql/data/home) ??? [TestLogger] 2025/08/14 16:09:18 ...s/storage/storage.go:157:initAttachments() [I] Initialising Attachment storage with type: minio ??? [TestLogger] 2025/08/14 16:09:18 ...les/storage/minio.go:97:NewMinioStorage() [I] Creating Minio storage at localhost:9000:gitea with base path attachments/ ??? [TestLogger] 2025/08/14 16:09:18 ...s/storage/storage.go:147:initAvatars() [I] Initialising Avatar storage with type: minio ??? [TestLogger] 2025/08/14 16:09:18 ...les/storage/minio.go:97:NewMinioStorage() [I] Creating Minio storage at localhost:9000:gitea with base path avatars/ ??? [TestLogger] 2025/08/14 16:09:18 ...s/storage/storage.go:173:initRepoAvatars() [I] Initialising Repository Avatar storage with type: minio ??? [TestLogger] 2025/08/14 16:09:18 ...les/storage/minio.go:97:NewMinioStorage() [I] Creating Minio storage at localhost:9000:gitea with base path repo-avatars/ ??? [TestLogger] 2025/08/14 16:09:18 ...s/storage/storage.go:167:initLFS() [I] Initialising LFS storage with type: minio ??? [TestLogger] 2025/08/14 16:09:18 ...les/storage/minio.go:97:NewMinioStorage() [I] Creating Minio storage at localhost:9000:gitea with base path lfs/ ??? [TestLogger] 2025/08/14 16:09:18 ...s/storage/storage.go:179:initRepoArchives() [I] Initialising Repository Archive storage with type: minio ??? [TestLogger] 2025/08/14 16:09:18 ...les/storage/minio.go:97:NewMinioStorage() [I] Creating Minio storage at localhost:9000:gitea with base path repo-archive/ ??? [TestLogger] 2025/08/14 16:09:18 ...s/storage/storage.go:189:initPackages() [I] Initialising Packages storage with type: minio ??? [TestLogger] 2025/08/14 16:09:18 ...les/storage/minio.go:97:NewMinioStorage() [I] Creating Minio storage at localhost:9000:gitea with base path packages/ ??? [TestLogger] 2025/08/14 16:09:18 ...s/storage/storage.go:200:initActions() [I] Initialising Actions storage with type: minio ??? [TestLogger] 2025/08/14 16:09:18 ...les/storage/minio.go:97:NewMinioStorage() [I] Creating Minio storage at localhost:9000:gitea with base path actions_log/ ??? [TestLogger] 2025/08/14 16:09:18 ...s/storage/storage.go:204:initActions() [I] Initialising ActionsArtifacts storage with type: minio ??? [TestLogger] 2025/08/14 16:09:18 ...les/storage/minio.go:97:NewMinioStorage() [I] Creating Minio storage at localhost:9000:gitea with base path actions_artifacts/ ??? [TestLogger] 2025/08/14 16:09:18 routers/common/db.go:23:InitDBEngine() [I] Beginning ORM engine initialization. ??? [TestLogger] 2025/08/14 16:09:18 routers/common/db.go:30:InitDBEngine() [I] ORM engine initialization attempt #1/10... ??? [TestLogger] 2025/08/14 16:09:20 routers/init.go:141:InitWebInstalled() [I] ORM engine initialization successful! ??? [TestLogger] 2025/08/14 16:09:20 ...er/issues/indexer.go:77:func1() [I] PID 211550: Initializing Issue Indexer: bleve ??? [TestLogger] 2025/08/14 16:09:20 ...exer/code/indexer.go:156:func3() [I] PID: 211550 Initializing Repository Indexer at: /home/earl-warren/software/forgejo/tests/integration/gitea-integration-pgsql/indexers/repos.bleve ??? [TestLogger] 2025/08/14 16:09:20 ...xer/stats/indexer.go:41:populateRepoIndexer() [I] Populating the repo stats indexer with existing repositories ??? [TestLogger] 2025/08/14 16:09:20 ...er/issues/indexer.go:154:func2() [I] Issue Indexer Initialization took 4.004266ms ??? [TestLogger] 2025/08/14 16:09:20 ...exer/code/indexer.go:231:func4() [I] Repository Indexer Initialization took 4.212531ms ??? [TestLogger] 2025/08/14 16:09:20 ...exer/code/indexer.go:257:populateRepoIndexer() [I] Populating the repo indexer with existing repositories ??? [TestLogger] 2025/08/14 16:09:20 routers/init.go:84:syncAppConfForGit() [I] AppPath changed from '' to '/home/earl-warren/software/forgejo/gitea' ??? [TestLogger] 2025/08/14 16:09:20 routers/init.go:89:syncAppConfForGit() [I] CustomConf changed from '' to '/home/earl-warren/software/forgejo/tests/pgsql.ini' ??? [TestLogger] 2025/08/14 16:09:20 routers/init.go:95:syncAppConfForGit() [I] re-sync repository hooks ... ??? [TestLogger] 2025/08/14 16:09:20 routers/init.go:98:syncAppConfForGit() [I] re-write ssh public keys ... ??? [TestLogger] 2025/08/14 16:09:20 modules/ssh/ssh.go:332:Listen() [I] Adding SSH host key: /home/earl-warren/software/forgejo/tests/integration/gitea-integration-pgsql/data/ssh/gitea.rsa ??? [TestLogger] 2025/08/14 16:09:20 modules/ssh/init.go:26:Init() [I] SSH server started on localhost:2202. Cipher list ([chacha20-poly1305@openssh.com aes128-ctr aes192-ctr aes256-ctr aes128-gcm@openssh.com aes256-gcm@openssh.com]), key exchange algorithms ([mlkem768x25519-sha256 curve25519-sha256 ecdh-sha2-nistp256 ecdh-sha2-nistp384 ecdh-sha2-nistp521 diffie-hellman-group14-sha256 diffie-hellman-group14-sha1]), MACs ([hmac-sha2-256-etm@openssh.com hmac-sha2-256 hmac-sha1]) ??? [TestLogger] 2025/08/14 16:09:20 ...s/graceful/server.go:50:NewServer() [I] Starting new SSH server: tcp:localhost:2202 on PID: 211550 ??? [TestLogger] 2025/08/14 16:09:20 ...er/cleanup_sha256.go:27:CleanupSHA256() [I] Start to cleanup dangling images with a sha256:* version ??? [TestLogger] 2025/08/14 16:09:20 ...er/cleanup_sha256.go:106:cleanupSHA256() [I] Nothing to cleanup ??? [TestLogger] 2025/08/14 16:09:20 ...er/cleanup_sha256.go:29:CleanupSHA256() [I] Finished to cleanup dangling images with a sha256:* version === TestValidateAndBufferAsset (tests/integration/api_repo_migrate_releases_s3_test.go:51) PASS ```
pat-s commented 2025-08-14 17:55:27 +02:00
Author
Member
Copy link

I ran the tests locally with the change reverted

Confused about this. How can you run the new test with changes reverted? It only works with the new added function.

Also I don't think that no-test failures in current HEAD are a proof that everything is OK as the test might just not catch the effective real world behavior. As this is about race conditions between download/upload, this is also not straightforward to replicate imo.

I ran the tests locally with the change reverted and it passes.

Are you fully sure the local minio test setup reliably mimics a S3 upload scenario?

Anyhow, I am out of time and ideas for this one, also with respect to proofing this in tests. The test I added proofs that the new logic works per se.
All I can say that is I can reliably reproduce it on a v12 instance that uses S3 for asset storage.

As I am building from source anyhow, I can also live with my local fix for the time being.

> I ran the tests locally with the change reverted Confused about this. How can you run the new test with changes reverted? It only works with the new added function. Also I don't think that no-test failures in current HEAD are a proof that everything is OK as the test might just not catch the effective real world behavior. As this is about race conditions between download/upload, this is also not straightforward to replicate imo. > I ran the tests locally with the change reverted and it passes. Are you fully sure the local minio test setup reliably mimics a S3 upload scenario? Anyhow, I am out of time and ideas for this one, also with respect to proofing this in tests. The test I added proofs that the new logic works per se. All I can say that is I can reliably reproduce it on a v12 instance that uses S3 for asset storage. As I am building from source anyhow, I can also live with my local fix for the time being.
earl-warren commented 2025-08-15 09:00:42 +02:00
Contributor
Copy link

#8906 is what I mean by reverting the change.

The test I added proofs that the new logic works per se.

It does, there is no doubt.

All I can say that is I can reliably reproduce it on a v12 instance that uses S3 for asset storage.

I want to see that as well. Since the integration tests you added work with or without the fix, there is something else going on.

What can I do to see that problem for myself?

https://codeberg.org/forgejo/forgejo/pulls/8906 is what I mean by reverting the change. > The test I added proofs that the new logic works per se. It does, there is no doubt. > All I can say that is I can reliably reproduce it on a v12 instance that uses S3 for asset storage. I want to see that as well. Since the integration tests you added work with or without the fix, there is something else going on. What can I do to see that problem for myself?
pat-s commented 2025-08-15 09:52:31 +02:00
Author
Member
Copy link

What can I do to see that problem for myself?

If you have a toy instance and access to S3-like storage, configure it to use it and run a migration on a repo which contains release assets (as these would be then pushed to the S3 bucket during the migration).

> What can I do to see that problem for myself? If you have a toy instance and access to S3-like storage, configure it to use it and run a migration on a repo which contains release assets (as these would be then pushed to the S3 bucket during the migration).
earl-warren commented 2025-08-15 10:31:59 +02:00
Contributor
Copy link

It works for me.

2025/08/15 10:26:48 cmd/web.go:114:showWebStartupMessage() [I] Forgejo version: 13.0.0-dev-302-5df8aac5f1+gitea-1.22.0 built with GNU Make 4.3, go1.24.6 : sqlite, sqlite_unlock_notify
2025/08/15 10:26:48 cmd/web.go:115:showWebStartupMessage() [I] * RunMode: dev
2025/08/15 10:26:48 cmd/web.go:116:showWebStartupMessage() [I] * AppPath: /home/earl-warren/software/forgejo/gitea
2025/08/15 10:26:48 cmd/web.go:117:showWebStartupMessage() [I] * WorkPath: /home/earl-warren/software/forgejo
2025/08/15 10:26:48 cmd/web.go:118:showWebStartupMessage() [I] * CustomPath: /home/earl-warren/software/forgejo/custom
2025/08/15 10:26:48 cmd/web.go:119:showWebStartupMessage() [I] * ConfigFile: /home/earl-warren/software/forgejo/custom/conf/app.ini
2025/08/15 10:26:48 cmd/web.go:120:showWebStartupMessage() [I] Prepare to run web server
2025/08/15 10:26:48 routers/init.go:115:InitWebInstalled() [I] Git version: 2.43.0, Wire Protocol Version 2 Enabled (home: /home/earl-warren/software/forgejo/data/home)
2025/08/15 10:26:48 ...s/storage/storage.go:157:initAttachments() [I] Initialising Attachment storage with type: minio
2025/08/15 10:26:48 ...les/storage/minio.go:97:NewMinioStorage() [I] Creating Minio storage at localhost:9000:gitea with base path attachments/
2025/08/15 10:26:48 ...s/storage/storage.go:147:initAvatars() [I] Initialising Avatar storage with type: minio
2025/08/15 10:26:48 ...les/storage/minio.go:97:NewMinioStorage() [I] Creating Minio storage at localhost:9000:gitea with base path avatars/
2025/08/15 10:26:48 ...s/storage/storage.go:173:initRepoAvatars() [I] Initialising Repository Avatar storage with type: minio
2025/08/15 10:26:48 ...les/storage/minio.go:97:NewMinioStorage() [I] Creating Minio storage at localhost:9000:gitea with base path repo-avatars/
2025/08/15 10:26:48 ...s/storage/storage.go:167:initLFS() [I] Initialising LFS storage with type: minio
2025/08/15 10:26:48 ...les/storage/minio.go:97:NewMinioStorage() [I] Creating Minio storage at localhost:9000:gitea with base path lfs/
2025/08/15 10:26:48 ...s/storage/storage.go:179:initRepoArchives() [I] Initialising Repository Archive storage with type: minio
2025/08/15 10:26:48 ...les/storage/minio.go:97:NewMinioStorage() [I] Creating Minio storage at localhost:9000:gitea with base path repo-archive/
2025/08/15 10:26:48 ...s/storage/storage.go:189:initPackages() [I] Initialising Packages storage with type: minio
2025/08/15 10:26:48 ...les/storage/minio.go:97:NewMinioStorage() [I] Creating Minio storage at localhost:9000:gitea with base path packages/
2025/08/15 10:26:48 ...s/storage/storage.go:200:initActions() [I] Initialising Actions storage with type: minio
2025/08/15 10:26:48 ...les/storage/minio.go:97:NewMinioStorage() [I] Creating Minio storage at localhost:9000:gitea with base path actions_log/
2025/08/15 10:26:48 ...s/storage/storage.go:204:initActions() [I] Initialising ActionsArtifacts storage with type: minio
2025/08/15 10:26:48 ...les/storage/minio.go:97:NewMinioStorage() [I] Creating Minio storage at localhost:9000:gitea with base path actions_artifacts/
...

Migrated https://code.forgejo.org/forgejo/release-notes-assistant (with "Releases" box checked). It completed and I get:

image

It works for me. ``` 2025/08/15 10:26:48 cmd/web.go:114:showWebStartupMessage() [I] Forgejo version: 13.0.0-dev-302-5df8aac5f1+gitea-1.22.0 built with GNU Make 4.3, go1.24.6 : sqlite, sqlite_unlock_notify 2025/08/15 10:26:48 cmd/web.go:115:showWebStartupMessage() [I] * RunMode: dev 2025/08/15 10:26:48 cmd/web.go:116:showWebStartupMessage() [I] * AppPath: /home/earl-warren/software/forgejo/gitea 2025/08/15 10:26:48 cmd/web.go:117:showWebStartupMessage() [I] * WorkPath: /home/earl-warren/software/forgejo 2025/08/15 10:26:48 cmd/web.go:118:showWebStartupMessage() [I] * CustomPath: /home/earl-warren/software/forgejo/custom 2025/08/15 10:26:48 cmd/web.go:119:showWebStartupMessage() [I] * ConfigFile: /home/earl-warren/software/forgejo/custom/conf/app.ini 2025/08/15 10:26:48 cmd/web.go:120:showWebStartupMessage() [I] Prepare to run web server 2025/08/15 10:26:48 routers/init.go:115:InitWebInstalled() [I] Git version: 2.43.0, Wire Protocol Version 2 Enabled (home: /home/earl-warren/software/forgejo/data/home) 2025/08/15 10:26:48 ...s/storage/storage.go:157:initAttachments() [I] Initialising Attachment storage with type: minio 2025/08/15 10:26:48 ...les/storage/minio.go:97:NewMinioStorage() [I] Creating Minio storage at localhost:9000:gitea with base path attachments/ 2025/08/15 10:26:48 ...s/storage/storage.go:147:initAvatars() [I] Initialising Avatar storage with type: minio 2025/08/15 10:26:48 ...les/storage/minio.go:97:NewMinioStorage() [I] Creating Minio storage at localhost:9000:gitea with base path avatars/ 2025/08/15 10:26:48 ...s/storage/storage.go:173:initRepoAvatars() [I] Initialising Repository Avatar storage with type: minio 2025/08/15 10:26:48 ...les/storage/minio.go:97:NewMinioStorage() [I] Creating Minio storage at localhost:9000:gitea with base path repo-avatars/ 2025/08/15 10:26:48 ...s/storage/storage.go:167:initLFS() [I] Initialising LFS storage with type: minio 2025/08/15 10:26:48 ...les/storage/minio.go:97:NewMinioStorage() [I] Creating Minio storage at localhost:9000:gitea with base path lfs/ 2025/08/15 10:26:48 ...s/storage/storage.go:179:initRepoArchives() [I] Initialising Repository Archive storage with type: minio 2025/08/15 10:26:48 ...les/storage/minio.go:97:NewMinioStorage() [I] Creating Minio storage at localhost:9000:gitea with base path repo-archive/ 2025/08/15 10:26:48 ...s/storage/storage.go:189:initPackages() [I] Initialising Packages storage with type: minio 2025/08/15 10:26:48 ...les/storage/minio.go:97:NewMinioStorage() [I] Creating Minio storage at localhost:9000:gitea with base path packages/ 2025/08/15 10:26:48 ...s/storage/storage.go:200:initActions() [I] Initialising Actions storage with type: minio 2025/08/15 10:26:48 ...les/storage/minio.go:97:NewMinioStorage() [I] Creating Minio storage at localhost:9000:gitea with base path actions_log/ 2025/08/15 10:26:48 ...s/storage/storage.go:204:initActions() [I] Initialising ActionsArtifacts storage with type: minio 2025/08/15 10:26:48 ...les/storage/minio.go:97:NewMinioStorage() [I] Creating Minio storage at localhost:9000:gitea with base path actions_artifacts/ ... ``` Migrated https://code.forgejo.org/forgejo/release-notes-assistant (with "Releases" box checked). It completed and I get: ![image](/attachments/259cb850-bdd3-4bd2-a952-f55000e3c930)
image.png
144 KiB
earl-warren commented 2025-08-15 10:43:52 +02:00
Contributor
Copy link

@pat-s wrote in #8865 (comment):

Anyhow, I am out of time and ideas for this one, also with respect to proofing this in tests. The test I added proofs that the new logic works per se. All I can say that is I can reliably reproduce it on a v12 instance that uses S3 for asset storage.

As I am building from source anyhow, I can also live with my local fix for the time being.

I appreciate that your time is precious. My time as a reviewer is also precious.

When you file a bug with no way to reproduce it while claiming it can be reproduced, I trust your word and I spend time figuring out what is wrong. I will not doubt that the test your wrote does not reproduce the problem, I will first look for the problem. Only when I don't figure it out will I revert the change to pinpoint exactly where the tests fails and get a better understanding of the problem. And when the test still passes, demonstrating that, after all, it is not a reproducer, the time I spent looking was wasted. It is the same when you ask me to spin a Forgejo instance and do a migration, claiming this is a reliable reproducer. When I do that and it works for me, that is also time I wasted.

When you provide test that is reproducer, it saves you and I valuable time.

As a conclusion, I suggest closing this pull request and opening an issue with "need more information". It can sit there as long as it need for someone else to run into the same problem that you have. It will provide valuable insight as to which environmental conditions are responsible for this behavior.

Could you please do that?

@pat-s wrote in https://codeberg.org/forgejo/forgejo/pulls/8865#issuecomment-6384664: > Anyhow, I am out of time and ideas for this one, also with respect to proofing this in tests. The test I added proofs that the new logic works per se. All I can say that is I can reliably reproduce it on a v12 instance that uses S3 for asset storage. > > As I am building from source anyhow, I can also live with my local fix for the time being. I appreciate that your time is precious. My time as a reviewer is also precious. When you file a bug with no way to reproduce it while claiming it can be reproduced, I trust your word and I spend time figuring out what is wrong. I will not doubt that the test your wrote does not reproduce the problem, I will first look for the problem. Only when I don't figure it out will I revert the change to pinpoint exactly where the tests fails and get a better understanding of the problem. And when the test still passes, demonstrating that, after all, it is not a reproducer, the time I spent looking was wasted. It is the same when you ask me to spin a Forgejo instance and do a migration, claiming this is a reliable reproducer. When I do that and it works for me, that is also time I wasted. When you provide test that is reproducer, it saves you and I valuable time. As a conclusion, I suggest closing this pull request and opening an issue with "need more information". It can sit there as long as it need for someone else to run into the same problem that you have. It will provide valuable insight as to which environmental conditions are responsible for this behavior. Could you please do that?
pat-s commented 2025-08-15 11:01:53 +02:00
Author
Member
Copy link

@earl-warren

I apologize overall as I was like 99% sure that this is reproducible, given I tried to outrule the influence of the reverse proxy or other local env-related matters.
Turns out it wasn't (given your tests, which I highly appreciate).

I understand that such cases are never easy and yes, I couldn't verify it 100% and was just "very sure", which is not enough.
I am still at loss for the actual reason, as I checked on the whole topic ~ 3-4h before even opening this PR, outruling many other parts before even attempting to patch the source, which then eventually worked.

It is the same when you ask me to spin a Forgejo instance and do a migration, claiming this is a reliable reproducer. When I do that and it works for me, that is also time I wasted.

I understand that, yet I hope you wouldn't have to do everything from scratch but just make a "quick edit" to the app.ini and referencing a S3 bucket there. But yes, I also could have done that with another instance of mine. Again, apologies. The best outcome would have been if anyone else with an existing S3 could have tried a migration call and reported this here.

Anyhow, I am still at loss which component in the overall process flow would be the culprit here. I don't see another one besides the reverse proxy taking major action in it, maybe I did a bad job outruling its influence.

@earl-warren I apologize overall as I was like 99% sure that this is reproducible, given I tried to outrule the influence of the reverse proxy or other local env-related matters. Turns out it wasn't (given your tests, which I highly appreciate). I understand that such cases are never easy and yes, I couldn't verify it 100% and was just "very sure", which is not enough. I am still at loss for the actual reason, as I checked on the whole topic ~ 3-4h before even opening this PR, outruling many other parts before even attempting to patch the source, which then eventually worked. > It is the same when you ask me to spin a Forgejo instance and do a migration, claiming this is a reliable reproducer. When I do that and it works for me, that is also time I wasted. I understand that, yet I hope you wouldn't have to do everything from scratch but just make a "quick edit" to the app.ini and referencing a S3 bucket there. But yes, I also could have done that with another instance of mine. Again, apologies. The best outcome would have been if anyone else with an existing S3 could have tried a migration call and reported this here. Anyhow, I am still at loss which component in the overall process flow would be the culprit here. I don't see another one besides the reverse proxy taking major action in it, maybe I did a bad job outruling its influence.
pat-s commented 2025-08-15 11:09:59 +02:00
Author
Member
Copy link

One (minor) issue I am facing now is: even though the issue is very likely env related, the fix proposed here would actually fix it (and also not break other existing functionality), so it could also be included, also because it has tests. It seems to be a more conservative approach to the process overall then the current implementation. Yes, I know that such action is potential dangerous as always something could break and that the patch proposed might also result in worse performance for this task, but yeah - the current implementation is at least more vulnerable to env-related settings which give admins a hard time to even relate to this issue in the first place (including potential frustration like "this software is not ready yet") (not saying that this would be my take on it, ofc).

Until I've figured out (if this will ever happen), which component might be causing this (e.g. a size-related constrain on the proxy), I am bound to carry this manual patch along with me in my instances, which is somewhat of a burden :/

One (minor) issue I am facing now is: even though the issue is very likely env related, the fix proposed here would actually fix it (and also not break other existing functionality), so it *could* also be included, also because it has tests. It seems to be a more conservative approach to the process overall then the current implementation. Yes, I know that such action is potential dangerous as always something could break and that the patch proposed might also result in worse performance for this task, but yeah - the current implementation is at least more vulnerable to env-related settings which give admins a hard time to even relate to this issue in the first place (including potential frustration like "this software is not ready yet") (not saying that this would be my take on it, ofc). Until I've figured out (if this will ever happen), which component might be causing this (e.g. a size-related constrain on the proxy), I am bound to carry this manual patch along with me in my instances, which is somewhat of a burden :/
pat-s closed this pull request 2025-08-15 11:12:28 +02:00
forgejo-moderation commented 2025-08-16 21:00:33 +02:00
Owner
Copy link

A Message from an account created to circumvent a ban from Forgejo spaces were deleted, please get in touch with the moderation team for more information or checkout the moderation process.

A Message from an account created to circumvent a ban from Forgejo spaces were deleted, please [get in touch with the moderation team](https://codeberg.org/forgejo/governance/src/branch/main/MODERATION-PROCESS.md#moderation-contact) for more information or checkout the [moderation process](https://codeberg.org/forgejo/governance/src/branch/main/MODERATION-PROCESS.md).
All checks were successful
requirements / merge-conditions (pull_request) Successful in 1s
Details
issue-labels / release-notes (pull_request_target) Has been skipped
Details
testing / frontend-checks (pull_request) Successful in 1m5s
Required
Details
testing / backend-checks (pull_request) Successful in 2m56s
Required
Details
testing / test-unit (pull_request) Successful in 6m49s
Required
Details
testing / test-remote-cacher (redis) (pull_request) Successful in 2m25s
Details
testing / test-remote-cacher (valkey) (pull_request) Successful in 2m23s
Details
testing / test-remote-cacher (garnet) (pull_request) Successful in 2m24s
Details
testing / test-remote-cacher (redict) (pull_request) Successful in 2m26s
Details
testing / test-e2e (pull_request) Successful in 21m40s
Details
testing / test-mysql (pull_request) Successful in 23m25s
Required
Details
testing / test-sqlite (pull_request) Successful in 28m20s
Required
Details
testing / test-pgsql (pull_request) Successful in 31m18s
Required
Details
testing / security-check (pull_request) Successful in 50s
Details
issue-labels / backporting (pull_request_target) Has been skipped
Details
milestone / set (pull_request_target) Has been skipped
Details

Pull request closed

Please reopen this pull request to perform a merge.
Sign in to join this conversation.
Reviewers
No reviewers
Gusted
earl-warren
Labels
Clear labels   
arch
riscv64

Archived

  
backport/v1.19
Scheduled for backport to Forgejo v1.19

Archived

  
backport/v1.20
Scheduled for backport to Forgejo v1.20

Archived

  
backport/v1.21/forgejo
Scheduled for backport to Forgejo v1.21

Archived

  
backport/v10.0/forgejo
Automated backport to v10.0

Archived

  
backport/v11.0/forgejo
Automated backport to v11.0

  
backport/v12.0/forgejo
Automated backport to v12.0

Archived

  
backport/v13.0/forgejo
Automated backport to v13.0

Archived

  
backport/v14.0/forgejo
Automated backport to v14.0

Archived

  
backport/v15.0/forgejo
Automated backport to v15.0

  
backport/v7.0/forgejo
Scheduled for backport to Forgejo v7.0

Archived

  
backport/v8.0/forgejo
Scheduled for backport to Forgejo v8.0

Archived

  
backport/v9.0/forgejo
Scheduled for backport to Forgejo v9.0

Archived

  
breaking
The release containing this change is not backward compatible

  
bug
Something is not working

Archived

  
bug
confirmed
 it can be reproduced

  
bug
duplicate
 bug has already been reported in the Forgejo tracker

  
bug
needs-more-info
 the information provided does not contain enough details

  
bug
new-report
 bug has just been reported and need triage (default label on issue creation)

  
bug
reported-upstream
 bug cannot be fixed within Forgejo easily, it has been reported upstream

  
code/actions
Forgejo Actions feature

  
code/api
API

  
code/auth
Forgejo Authentication

  
code/auth/faidp
Forgejo as Identity Provider (in OAuth/OIDC flow)

  
code/auth/farp
Forgejo as Relying Party / Client (in OAuth/OIDC flow)

  
code/email
Everything related to email in Forgejo

  
code/federation
Federation

  
code/git
Related to the Git backend in Forgejo

  
code/migrations
Migration between Git forges (i.e. for GitHub, GitLab, Gitea, Forgejo, etc.). NOT for database migrations.

  
code/packages
Forgejo package and container registry

  
code/wiki
  
database
MySQL
 https://www.mysql.com/

  
database
PostgreSQL
 https://www.postgresql.org/

  
database
SQLite
 https://sqlite.org/

  
dependency-upgrade
https://codeberg.org/forgejo/forgejo/issues/2779

  
dependency
Chi
 https://github.com/go-chi/chi/

Archived

  
dependency
Chroma
 https://github.com/alecthomas/chroma/

Archived

  
dependency
F3
 https://f3.forgefriends.org/

  
dependency
ForgeFed
 https://forgefed.org

  
dependency
garage
 https://git.deuxfleurs.fr/Deuxfleurs/garage

  
dependency
Gitea
 https://github.com/go-gitea/gitea

Archived

  
dependency
Golang
 https://go.dev/

  
Discussion

   
duplicate
This issue or pull request already exists

  
enhancement/feature
New feature

  
forgejo/accessibility
Accessibility (a11y)

  
forgejo/branding
Branding (logo, name, tagline etc.)

  
forgejo/ci
Forgejo Actions CI configuration

  
forgejo/commit-graph
The commit graph feature and page.

  
forgejo/documentation

   
forgejo/furnace cleanup
Keeping Forgejo in sync with its dependencies and contributing back to them

Archived

  
forgejo/i18n
t9n/translation, l10n/localization, and i18n/internationalization of Forgejo

  
forgejo/interop
Interoperability with other services: Webhooks, bridges, integrations

  
forgejo/moderation
Moderation

  
forgejo/privacy
Privacy first

  
forgejo/release
Release management

  
forgejo/scaling
Performance and scaling

  
forgejo/security
Security (please disclose responsibly)

  
forgejo/ui
User interface

  
Gain
High
 User research provides indicators that this would be good to have, interested contributors are encouraged to pick this.

  
Gain
Nice to have
 This is likely worth having, but the assumption is not backed by user research data (it might benefit a small amount of users only.) Unlikely to receive much attention, but feel free to pick.

  
Gain
Undefined
 Not enough information to assess the request's benefits. This issue may be closed if no gain is established: You can help by giving us more input.

  
Gain
Very High
 User research indicates that this is an important improvement for Forgejo users. Contributions very welcome!

  
good first issue
Optimal for first-timers! Make sure to look for further explanations and ask for help if needed. If you want, you can consider the person who added this label as a point of contact.

  
i18n/backport-stable
This PR needs to be backported to stable branch of Forgejo safely and manually, using a migration script.

  
impact
large
 Large impact: Potential data loss, many users affected, major degradation in UX.

  
impact
medium
 Medium impact: Several users affected, degradation in UX, workarounds might be available but inconvenient.

  
impact
small
 Small impact: No data loss, workarounds might be available, affects few users.

  
impact
unknown
 Report was not yet triaged to assess impact.

  
Incompatible license
This pull request contains changes that are not (yet) compatible with the current Forgejo license

  
issue
closed
 The issue was resolved in the repository of the dependency

  
issue
do-not-exist-yet
 An issue should be created in the respository of the dependency

  
issue
open
 An open issue exists in the upstream repository of the dependency

  
manual test
Pull requests that have been merged with a manual test

Archived

  
Manually tested during feature freeze
The manual test instructions were followed

  
OS
FreeBSD
 Specific to the FreeBSD Operating System

  
OS
Linux
 Specific to (GNU/)Linux Operating Systems

  
OS
macOS
 Specific to the MacOS Operating System

  
OS
Windows
 Specific to the Windows Operating System

  
problem
A user report about a problem. Needs to be triaged to find potential solutions.

  
QA

   
regression
found in the version of the milestone and not before

  
release blocker
Issues that must be fixed before the release can be published

  
Release Cycle
Feature Freeze
 Only bug fixes with automated tests (except for CSS/JavaScript)

  
release-blocker
v7.0
 Issues that must be fixed before Forgejo v7.0 can be released 17 April 2024

Archived

  
release-blocker
v7.0.1
 Issues that must be fixed before Forgejo v7.0.1 can be released

Archived

  
release-blocker
v7.0.2
 Issues that must be fixed before Forgejo v7.0.2 can be released

Archived

  
release-blocker
v7.0.3
 Issues that must be fixed before Forgejo v7.0.3 can be released

Archived

  
release-blocker
v7.0.4
 Issues that must be fixed before Forgejo v7.0.4 can be released

Archived

  
release-blocker
v8.0.0
 Issues that must be fixed before Forgejo v8.0.0 can be released

Archived

  
release-blocker/v9.0.0
Issues that must be fixed before Forgejo v9.0.0 can be released

Archived

  
run-all-playwright-tests
Add this label to a PR to run all playwright tests manually.

  
run-end-to-end-tests
Trigger additional tests on the PR when it is ready to be merged

  
stage
2-research
 Needs research to moe on.

  
stage
3-design
 Needs design to move on

  
stage
4-implementation
 This is actionable and waits for implementation

  
test
manual
 manual testing has been documented

  
test
needed
 test should be added

  
test
needs-help
 help needed to add a test

  
test
not-needed
 no additional test is needed

  
test
present
 test has been added

  
untested
Pull requests that have been merged with no test and submitted as is to the dependency where they belong

Archived

  
User research - time-tracker
Time tracking feature for issues and the JS stopwatch.

  
valuable code
This PR was closed because the implementation is incomplete

  
worth a release-note
Add this PR to the release notes

  
User research - Accessibility
Requires input about accessibility features, likely involves user testing.

  
User research - Blocked
Do not pick as-is! We are happy if you can help, but please coordinate with ongoing redesign in this area.

  
User research - Community
Community features, such as discovering other people's work or otherwise feeling welcome on a Forgejo instance.

  
User research - Config (instance)
Instance-wide configuration, authentication and other admin-only needs.

  
User research - Errors
How to deal with errors in the application and write helpful error messages.

  
User research - Filters
How filter and search is being worked with.

  
User research - Future backlog
The issue might be inspiring for future design work.

  
User research - Git workflow
AGit, fork-based and new Git workflow, PR creation etc

  
User research - Labels
Active research about Labels

  
User research - Moderation
Moderation Featuers for Admins are undergoing active User Research

  
User research - Needs input
Use this label to let the User Research team know their input is requested.

  
User research - Notifications/Dashboard
Research on how users should know what to do next.

  
User research - Rendering
Text rendering, markup languages etc

  
User research - Repo creation
Active research about the New Repo dialog.

  
User research - Repo units
The repo sections, disabling them and the "Add more" button.

  
User research - Security

   
User research - Settings (in-app)
How to structure in-app settings in the future?

No labels
arch
riscv64
backport/v1.19
backport/v1.20
backport/v1.21/forgejo
backport/v10.0/forgejo
backport/v11.0/forgejo
backport/v12.0/forgejo
backport/v13.0/forgejo
backport/v14.0/forgejo
backport/v15.0/forgejo
backport/v7.0/forgejo
backport/v8.0/forgejo
backport/v9.0/forgejo
breaking
bug
bug
confirmed
bug
duplicate
bug
needs-more-info
bug
new-report
bug
reported-upstream
code/actions
code/api
code/auth
code/auth/faidp
code/auth/farp
code/email
code/federation
code/git
code/migrations
code/packages
code/wiki
database
MySQL
database
PostgreSQL
database
SQLite
dependency-upgrade
dependency
Chi
dependency
Chroma
dependency
F3
dependency
ForgeFed
dependency
garage
dependency
Gitea
dependency
Golang
Discussion
duplicate
enhancement/feature
forgejo/accessibility
forgejo/branding
forgejo/ci
forgejo/commit-graph
forgejo/documentation
forgejo/furnace cleanup
forgejo/i18n
forgejo/interop
forgejo/moderation
forgejo/privacy
forgejo/release
forgejo/scaling
forgejo/security
forgejo/ui
Gain
High
Gain
Nice to have
Gain
Undefined
Gain
Very High
good first issue
i18n/backport-stable
impact
large
impact
medium
impact
small
impact
unknown
Incompatible license
issue
closed
issue
do-not-exist-yet
issue
open
manual test
Manually tested during feature freeze
OS
FreeBSD
OS
Linux
OS
macOS
OS
Windows
problem
QA
regression
release blocker
Release Cycle
Feature Freeze
release-blocker
v7.0
release-blocker
v7.0.1
release-blocker
v7.0.2
release-blocker
v7.0.3
release-blocker
v7.0.4
release-blocker
v8.0.0
release-blocker/v9.0.0
run-all-playwright-tests
run-end-to-end-tests
stage
2-research
stage
3-design
stage
4-implementation
test
manual
test
needed
test
needs-help
test
not-needed
test
present
untested
User research - time-tracker
valuable code
worth a release-note
User research - Accessibility
User research - Blocked
User research - Community
User research - Config (instance)
User research - Errors
User research - Filters
User research - Future backlog
User research - Git workflow
User research - Labels
User research - Moderation
User research - Needs input
User research - Notifications/Dashboard
User research - Rendering
User research - Repo creation
User research - Repo units
User research - Security
User research - Settings (in-app)
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
3 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
forgejo/forgejo!8865
No description provided.