Skip to content

fix(output): synchronize concurrent writes to the prefixed writer - #2983

Open
ikshantshukla123 wants to merge 1 commit into
go-task:mainfrom
ikshantshukla123:fix/prefixed-output-concurrent-write-panic
Open

fix(output): synchronize concurrent writes to the prefixed writer#2983
ikshantshukla123 wants to merge 1 commit into
go-task:mainfrom
ikshantshukla123:fix/prefixed-output-concurrent-write-panic

Conversation

@ikshantshukla123

@ikshantshukla123 ikshantshukla123 commented Aug 16, 2026

Copy link
Copy Markdown
Contributor

Description #2945

This pr fixes a panic that can happen with output: prefixed mode. when a task writes to both stdout and stderr at the same time, the same prefixWriter ends up being used from two goroutines (os/exec copies each stream from its own goroutine) the shared bytes.Buffer is not safe for concurrent use, so this could corrupt its state and crash with a "slice bounds out of range" panic when their is multiple stdout and stderr concurrently.I added a mutex to prefixWriter and lock it in both Write and Close so the buffer is only ever touched by one goroutine at a time.
I also added a regression test that writes 1000 lines to stdout and 1000 to stderr concurrently, then checks that we get exactly 2000 correctly prefixed lines back. I confirmed it panics without the fix, and passes with it (also clean under -race).

Notes

Checklist

  • I have read and followed the Contribution Guide.
  • I have disclosed the use of any AI generated content in this pull
    request per the AI Usage Policy.
  • I fully understand the changes and have hand written the description
    (No AI) of this pull request.

and also I have used strings.Split as i found it is used across the repo in (e.g. task_test.go, watch_test.go etc).and it is a completely standard import for test assertions. output_test.go didn't need it before only because the existing tests there used assert. Equal on exact strings rather than splitting or parsing output.

Ready for further discussions.

WrapWriter uses the same prefixWriter for both stdout and stderr, and os/exec can write to both streams concurrently. Since bytes.Buffer is not safe for concurrent use, this could corrupt its state and cause a "slice bounds out of range" panic. Add a mutex to prefixWriter and lock access in both Write and Close so only one goroutine uses the buffer at a time. Also add a regression test that writes many lines to stdout and stderr concurrently and verifies every line is correctly prefixed; it fails on the unfixed code and is reliably detected with -race.

@trulede trulede left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test might be useful during development but is otherwise useless. Why did you include it again?

@ikshantshukla123

Copy link
Copy Markdown
Contributor Author

The test might be useful during development but is otherwise useless. Why did you include it again?

okay okay apology you already flagged this in #2949 and I missed it. I will limit the test when it runs like I will move the concurrency test into its own file gated with go:build race, so it only executes under race and never runs in plain CI.
should i go with this or just drop the test?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants