perf: write captured stdout/stderr straight to the output file - #2973
Merged
Conversation
acozzette
marked this pull request as ready for review
August 14, 2026 15:25
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 61ebeeccf3
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
acozzette
force-pushed
the
stdout-stderr-capture
branch
from
August 14, 2026 16:55
61ebeec to
30e8f6d
Compare
jbedard
reviewed
Aug 14, 2026
jbedard
reviewed
Aug 14, 2026
Pure code motion in the js_binary launcher, split out to keep the following commit reviewable. The --bazel-bindir block and the resolve_capture_path helper with its three call sites move up to the top of the script, ahead of the stdout/stderr capture setup, so that the absolute capture paths are known before anything is logged. Neither block depends on runfiles initialization, and resolve_capture_path still resolves against $PWD before any cd, so the values it produces are unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The launcher buffered node's stdout and stderr in mktemp files and only copied them to the js_run_binary(stdout, stderr) outputs from an EXIT trap. Because work had to happen after node exited, the launcher could not exec node: it backgrounded node, installed signal forwarding traps and waited on it, keeping an extra shell alive for the whole action and proxying signals and terminal control through it. The exec fast path was in practice dead code for build actions, since js_run_binary(silent_on_success) defaults to True and that alone forced both temp captures to be allocated. Point the capture at the declared output file and let node write it directly. A mktemp is now used only when silent_on_success is set and there is no output file to write to, which is the only case left that needs post-processing; it is flagged with STD*_CAPTURE_IS_TEMP and is the only thing that blocks the exec. A stream captured to an output file is therefore no longer echoed back, neither on a build failure nor when silent_on_success is False. Bazel discards the output file of a failed action, so launcher FATAL and ERROR diagnostics now go to the real stderr instead of into that file; INFO and DEBUG logs stay with the captured stderr they annotate. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
acozzette
force-pushed
the
stdout-stderr-capture
branch
from
August 14, 2026 20:27
dca6790 to
9afffdb
Compare
jbedard
approved these changes
Aug 14, 2026
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
2 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.
It is advantageous to write the captured stderr or stdout straight to its intended destination. This eliminates a cleanup step after Node exits, making it possible to invoke Node via
execand thus avoid forking an extra process.There is a drawback, which is that if the action fails then the captured output will be lost, making it somewhat harder to troubleshoot. This seems like an acceptable cost given the performance benefit, though.
One other caveat is that we can only do this when
silent_on_successis not enabled, but that parameter defaults toTrueforjs_run_binary. Under the default behavior, we have to store stdout and stderr in temporary files so that we are prepared to print them out if the action fails. So to get any improvement from this change, the action has to opt in by settingsilent_on_success = False, or else invoke thejs_binarywithout usingjs_run_binary.This commit makes one other significant change, which is to always print out the launcher script's fatal- and error-level log messages instead of capturing them. This behavior is logical because if such a message is getting logged then the action is almost certainly going to fail, in which case there is no point in attempting to capture the log message anyway. We might as well print out the message to help the user understand what went wrong.
Changes are visible to end-users: no (except that build output may be different in some unusual cases when an action fails)
Test plan