Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Read long log lines from file storage correctly #4048

Merged
merged 2 commits into from
Aug 25, 2024

Conversation

hg
Copy link
Contributor

@hg hg commented Aug 18, 2024

Log lines may contain up to 1 MB of text, but with default settings bufio.Scanner only reads lines up to 64k in length and fails on anything larger, rendering the Scanner unusable.

As a result, the user won't see anything after the first very long line, including the line itself.

when:
  event: push

steps:
  logs:
    image: ubuntu:24.04
    commands:
      - seq 1 3
      - tr -dc '[:alnum:]' </dev/urandom | head -c70k
      - echo
      - seq 4 6

The output should be:

1
2
3
zkwmLrpYmk6XcgJZiYqzNO3hsIV26HJfVQfyvSo1ccrMqP ... very long line of random text
4
5
6

Instead, Woodpecker cuts everything after 3:

1
2
3
(end)

I tested this on jobs with up to 20 MBs of output (simply update 70k with something like 20M).

cli/exec/exec.go Outdated Show resolved Hide resolved
@hg hg force-pushed the file-long-logs branch from 10b3284 to 06b5675 Compare August 25, 2024 20:08
@6543
Copy link
Member

6543 commented Aug 25, 2024

please dont forcepush if you can! it makes reviews harder and we squashmerge anyway ...

@6543 6543 added server bug Something isn't working labels Aug 25, 2024
@6543 6543 enabled auto-merge (squash) August 25, 2024 20:33
@woodpecker-bot
Copy link
Contributor

woodpecker-bot commented Aug 25, 2024

Deployment of preview was torn down

@6543
Copy link
Member

6543 commented Aug 25, 2024

@6543
Copy link
Member

6543 commented Aug 25, 2024

thanks for not running at the dedicated pull :/

@6543 6543 disabled auto-merge August 25, 2024 20:49
@6543 6543 merged commit 37d1ca8 into woodpecker-ci:main Aug 25, 2024
5 of 7 checks passed
@woodpecker-bot woodpecker-bot mentioned this pull request Aug 25, 2024
1 task
@hg
Copy link
Contributor Author

hg commented Aug 25, 2024

Since this issue crops up in other pull requests too, it might be easier on you to mention it in the docs or in a pull request template (example).

@6543
Copy link
Member

6543 commented Aug 25, 2024

If you d'like to create a pull? 🙏

@hg
Copy link
Contributor Author

hg commented Aug 25, 2024

Sure, I'll have to think about proper messaging for a moment.

6543 added a commit that referenced this pull request Aug 25, 2024
because I got distracted by an unrelated CI error and thought the pull was ok :/
#4048 (comment)
6543 pushed a commit to 6543-forks/woodpecker that referenced this pull request Sep 5, 2024
6543 added a commit to 6543-forks/woodpecker that referenced this pull request Sep 5, 2024
because I got distracted by an unrelated CI error and thought the pull was ok :/
woodpecker-ci#4048 (comment)
@woodpecker-bot woodpecker-bot mentioned this pull request Sep 8, 2024
1 task
@woodpecker-bot woodpecker-bot mentioned this pull request Dec 14, 2024
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working server
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants