## Summary With native steps on the Docker executor, `Connect` starts `gitlab-runner-helper steps proxy` with `docker exec` and splits the attached stream with `stdcopy.StdCopy`: stdout goes into an `io.Pipe` that the step-runner client reads, and stderr goes into `omitwriter.New()` ([`executors/docker/steps.go#L136-159`](https://gitlab.com/gitlab-org/gitlab-runner/-/blob/a278fd0507debd103f90ce8a8c62ca4743a97a10/executors/docker/steps.go#L136-159)). `omitWriter.Write` declares a named result `n` and never assigns it, so it stores the bytes and returns `0, nil` ([`executors/docker/internal/omitwriter/omit_writer.go#L20-31`](https://gitlab.com/gitlab-org/gitlab-runner/-/blob/a278fd0507debd103f90ce8a8c62ca4743a97a10/executors/docker/internal/omitwriter/omit_writer.go#L20-31)). `StdCopy` treats a write that reports fewer bytes than the frame as `io.ErrShortWrite` ([moby `api/pkg/stdcopy/stdcopy.go#L136-L137` at `api/v1.56.0`](https://github.com/moby/moby/blob/api/v1.56.0/api/pkg/stdcopy/stdcopy.go#L136-L137), the version runner pins). So the first stderr frame from `steps proxy` stops the copy: the pipe is closed with `short write: <stderr text>`, and the stdout frames after it never reach the client. Fleeting carries the same writer as `stderrOmitWriter` and fixed the same bug in gitlab-org/fleeting/fleeting@ade46f38, merged with gitlab-org/fleeting/fleeting!117. #38930 reports that `short write` from fleeting's copy, for `instance_ready_command`, and runner main already pins a fleeting version that contains the fix. The Docker executor's own copy was not changed. ## Steps to reproduce No Docker daemon is needed. On main (a278fd05), add this test to `executors/docker/internal/omitwriter` and run `go test -v -run TestOmitWriterShortWrite ./executors/docker/internal/omitwriter/`: ```go package omitwriter import ( "bytes" "encoding/binary" "io" "strings" "testing" "github.com/moby/moby/api/pkg/stdcopy" ) func TestOmitWriterShortWrite(t *testing.T) { n, err := io.Copy(New(), strings.NewReader("some stderr output")) t.Logf("io.Copy: n=%d err=%v", n, err) // One stdout frame, one stderr frame, one stdout frame, as docker attach sends them. var stream bytes.Buffer for _, f := range []struct { kind stdcopy.StdType data string }{ {stdcopy.Stdout, "first|"}, {stdcopy.Stderr, "a line on stderr\n"}, {stdcopy.Stdout, "second|"}, } { hdr := make([]byte, 8) hdr[0] = byte(f.kind) binary.BigEndian.PutUint32(hdr[4:], uint32(len(f.data))) stream.Write(append(hdr, f.data...)) } var stdout bytes.Buffer _, err = stdcopy.StdCopy(&stdout, New(), &stream) t.Logf("StdCopy: stdout=%q err=%v", stdout.String(), err) } ``` It prints: ```plaintext io.Copy: n=0 err=short write StdCopy: stdout="first|" err=short write ``` In a job, this path is taken on the Docker executor whenever native steps are used (`run:` in the job, `FF_SCRIPT_TO_STEP_MIGRATION` or `FF_CONCRETE`) and `steps proxy` writes anything to stderr. I have not reproduced it end to end with a real job, so I am not adding a `.gitlab-ci.yml` here: `steps proxy` writes nothing to stderr while it works, so a plain job would not show it. ## Actual behavior - `omitWriter.Write` returns `0, nil` for any non-empty slice. - `io.Copy` and `stdcopy.StdCopy` into it fail with `io.ErrShortWrite` on the first write. - In `Connect`, the first stderr frame of `steps proxy` ends the copy goroutine, the pipe the step-runner client reads is closed with `short write: <stderr text>`, and the stdout frames after it are lost. Any output on stderr is enough, even a line that is not an error. ## Expected behavior `Write` returns `len(p), nil`, as `io.Writer` requires, and keeps the end of the stderr text (just under 32 KiB) for the error message as it does now. Output on stderr does not end the connection, and stdout keeps reaching the client. With `return len(p), nil`, the test above prints `io.Copy: n=18 err=<nil>` and `StdCopy: stdout="first|second|" err=<nil>`, and the existing `Test_OmitWriter` cases still pass. ## Relevant logs and/or screenshots The test output above. The existing `Test_OmitWriter` does not catch this because it discards the count `Write` returns ([`omit_writer_test.go#L45`](https://gitlab.com/gitlab-org/gitlab-runner/-/blob/a278fd0507debd103f90ce8a8c62ca4743a97a10/executors/docker/internal/omitwriter/omit_writer_test.go#L45)). ## Environment description `docker` executor with native steps (`docker-windows` turns native steps off). Not specific to a Docker version or a `config.toml` setting. ### Used GitLab Runner version GitLab Runner v19.5.0, whose `omit_writer.go` and `steps.go` are the same as at the commit below. main at a278fd05. The same code is in v19.4.1, and `Write` has returned the unassigned `n` since the writer was added in cb66a99d2. ## Possible fixes Return `len(p), nil` from `Write`, and assert the count in the tests: in the `Test_OmitWriter` table and through `io.Copy`, as fleeting did, plus one case through `stdcopy.StdCopy`, which is how `steps.go` uses the writer. One more change belongs with it. Today the stderr text of a `steps proxy` that fails (for example when it cannot dial the step-runner socket, logs the error and exits) reaches the client only because of the short write: `steps.go` attaches `stderr.Error()` only when `StdCopy` returns an error. Once `Write` is fixed, `StdCopy` reads to EOF and returns `nil`, and that text is dropped. So the copy goroutine should also report what stderr captured when the stream ends. Today `Error()` returns a non-nil error even when nothing was written; making it return `nil` for an empty buffer, as fleeting's copy does, keeps that check simple. I'd like to work on this.