fix: permit larger single-line output sizes to be parsed from FORGEJO_OUTPUT file #1359
No reviewers
aahlenst
Labels
No labels
FreeBSD
Kind/Breaking
Kind/Bug
Kind/Chore
Kind/DependencyUpdate
Kind/Documentation
Kind/Enhancement
Kind/Feature
Kind/Security
Kind/Testing
Priority
Critical
Priority
High
Priority
Low
Priority
Medium
Reviewed
Confirmed
Reviewed
Duplicate
Reviewed
Invalid
Reviewed
Won't Fix
Status
Abandoned
Status
Blocked
Status
Need More Info
Windows
linux-powerpc64le
linux-riscv64
linux-s390x
run-end-to-end-tests
run-forgejo-tests
No milestone
No project
No assignees
2 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
forgejo/runner!1359
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "mfenniak/forgejo-runner:parse-larger-outputs"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Forgejo is supposed to have a 1MB max output size (
codeberg.org/forgejo/forgejo@f24a97f719/routers/api/actions/runner/runner.go (L209-L213)), but the parsing ofFORGEJO_OUTPUTdidn't permit more than 64kB per-line (eg. "output=value" must be < 64kB).Fixes #1353.
384db1a504084ef24b14@ -25,6 +25,8 @@ func parseEnvFile(e Container, srcPath string, env *map[string]string) common.Exreturn err}s := bufio.NewScanner(reader)// Action outputs can be up to 1MB, plus the "key=" prefix and "\n" suffix, so allow 256b + 1MB max buffer sizeWould be good to link to the place that stipulates that outputs can be up to 1 MB. Facilitates code archaeology.
What about better distinguishing the output size from the buffer size? Example: "The maximum allowed output size is 1 MiB. The buffer must be 256 bytes bigger to account for the prefix (maximum of 255 bytes) and the
\nsuffix."Edit: Does the maximum length of the prefix include
=or not? If it does not, the buffer needs to be 1 byte bigger.I've updated the related comment, and added a test at both the key & value extremes to ensure the buffer is sized appropriately.
@ -26,2 +26,4 @@}s := bufio.NewScanner(reader)// Action outputs can be up to 1MB, plus the "key=" prefix and "\n" suffix, so allow 256b + 1MB max buffer sizes.Buffer(make([]byte, 0, 1024), 256+(1024*1024))Is there a way to emit a warning when the output is bigger, for example, by checking
Err()?Ah, this is a great idea. But I'm tempted to (and have updated the PR) to make this an error rather than a warning. What do you think? 🤔 My thought is that you've got a subtle data loss issue in an edge case -- an error that stops the workflow is better than a warning that could be missed.
@ -0,0 +46,4 @@name: "1MB value",rawContents: fmt.Sprintf("abc=%s\n", data1Mb.String()),mapContents: map[string]string{"abc": data1Mb.String()},},If I understand
codeberg.org/forgejo/forgejo@f24a97f719/routers/api/actions/runner/runner.go (L205-L208)correctly, the maximum key length is 255 characters. What about adding a test with a 255 characters long key?Sounds good.
084ef24b14778017036eLooks great now, thanks a lot!