feat: allow capturing full output to stdout, modernize API#9
Merged
talos-bot merged 2 commits intosiderolabs:mainfrom Feb 23, 2026
Merged
Conversation
6a4b132 to
f7d7790
Compare
Member
|
Nice catch! I wonder if we should not use circular buffer here at all. |
f7d7790 to
a9f9327
Compare
Contributor
Author
|
@smira as you like. Just tell me what you prefer. |
Member
|
Let me think about this a bit more, I certainly don't like the interface of this function, neither I like the idea of allocating 1 MiB for every command execution. |
Deprecate old functions, introduce a better extensible replacement with `RunWithOptions`. The stdout circbuf was sharing MaxStderrLen (4096 bytes) with stderr. Subprocess stdout can be arbitrarily large — for example a command that produces structured data (JSON, certificates, encoded blobs) can easily exceed 4096 bytes, resulting in silent mid-stream truncation with no error returned to the caller. Add MaxStdoutLen = 1 MiB and use it for the stdout buffer. MaxStderrLen remains at 4096 bytes since error output is naturally short. Add TestLargeStdout regression test that generates 6000 bytes of stdout and asserts the full output is returned untruncated. Signed-off-by: Mickaël Canévet <mickael.canevet@proton.ch> Signed-off-by: Andrey Smirnov <andrey.smirnov@siderolabs.com>
Rekres with latest. Signed-off-by: Andrey Smirnov <andrey.smirnov@siderolabs.com>
a9f9327 to
5f31ba9
Compare
Member
I did some refactoring, and I hope this API will be better and more clear. |
frezbo
reviewed
Feb 23, 2026
| module github.com/siderolabs/go-cmd | ||
|
|
||
| go 1.23.2 | ||
| go 1.24.0 |
Member
There was a problem hiding this comment.
why would we bump the minimum? we can backport more freely if we don't aggressively bump
frezbo
approved these changes
Feb 23, 2026
Member
|
/m |
smira
added a commit
to smira/talos
that referenced
this pull request
Feb 23, 2026
See: * siderolabs/go-blockdevice#147 * siderolabs/go-cmd#9 Lots of changes through the code as I deprecated `Run`, `RunWithContext` methods and allow only new `RunWithOptions` to clean up the library usage. Signed-off-by: Andrey Smirnov <andrey.smirnov@siderolabs.com>
smira
added a commit
to smira/talos
that referenced
this pull request
Mar 18, 2026
See: * siderolabs/go-blockdevice#147 * siderolabs/go-cmd#9 Lots of changes through the code as I deprecated `Run`, `RunWithContext` methods and allow only new `RunWithOptions` to clean up the library usage. Signed-off-by: Andrey Smirnov <andrey.smirnov@siderolabs.com> (cherry picked from commit 7cf1de2)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
RunContextallocates the stdout circbuf withMaxStderrLen(4096 bytes), the same constant used for stderr. A circbuf silently discards earlier bytes once the limit is reached, so any subprocess that writes more than 4 KiB to stdout gets its output truncated with no error returned to the caller.Stderr is naturally short (error messages), so 4 KiB is fine there. Stdout has no such bound — commands that emit structured data (JSON payloads, certificates, encoded blobs, etc.) can easily exceed 4 KiB.
Fix
Add
MaxStdoutLen = 1 MiBand use it for the stdout buffer.MaxStderrLenis unchanged.Test
TestLargeStdoutgenerates 6000 bytes of stdout (> old 4096-byte limit) and asserts the full output is returned untruncated.