Skip to content

Fix: cap aggregated exec output consistently#9759

Merged
jif-oai merged 16 commits into
openai:mainfrom
Kbediako:fix/exec-aggregated-output-cap
Jan 27, 2026
Merged

Fix: cap aggregated exec output consistently#9759
jif-oai merged 16 commits into
openai:mainfrom
Kbediako:fix/exec-aggregated-output-cap

Conversation

@Kbediako

@Kbediako Kbediako commented Jan 23, 2026

Copy link
Copy Markdown
Contributor

WHAT?

  • Bias aggregated output toward stderr under contention (2/3 stderr, 1/3 stdout) while keeping the 1 MiB cap.
  • Rebalance unused stderr share back to stdout when stderr is tiny to avoid underfilling.
  • Add tests for contention, small-stderr rebalance, and under-cap ordering (stdout then stderr).

WHY?

  • Review feedback requested stderr priority under contention.
  • Avoid underfilled aggregated output when stderr is small while preserving a consistent cap across exec paths.

HOW?

  • Update aggregate_output to compute stdout/stderr shares, then reassign unused capacity to the other stream.
  • Use the helper in both Windows and async exec paths.
  • Add regression tests for contention/rebalance and under-cap ordering.

BEFORE

// Best-effort aggregate: stdout then stderr (capped).
let mut aggregated = Vec::with_capacity(
    stdout
        .text
        .len()
        .saturating_add(stderr.text.len())
        .min(EXEC_OUTPUT_MAX_BYTES),
);
append_capped(&mut aggregated, &stdout.text, EXEC_OUTPUT_MAX_BYTES);
append_capped(&mut aggregated, &stderr.text, EXEC_OUTPUT_MAX_BYTES);
let aggregated_output = StreamOutput {
    text: aggregated,
    truncated_after_lines: None,
};

AFTER

fn aggregate_output(
    stdout: &StreamOutput<Vec<u8>>,
    stderr: &StreamOutput<Vec<u8>>,
) -> StreamOutput<Vec<u8>> {
    let total_len = stdout.text.len().saturating_add(stderr.text.len());
    let max_bytes = EXEC_OUTPUT_MAX_BYTES;
    let mut aggregated = Vec::with_capacity(total_len.min(max_bytes));

    if total_len <= max_bytes {
        aggregated.extend_from_slice(&stdout.text);
        aggregated.extend_from_slice(&stderr.text);
        return StreamOutput {
            text: aggregated,
            truncated_after_lines: None,
        };
    }

    // Under contention, reserve 1/3 for stdout and 2/3 for stderr; rebalance unused stderr to stdout.
    let want_stdout = stdout.text.len().min(max_bytes / 3);
    let want_stderr = stderr.text.len();
    let stderr_take = want_stderr.min(max_bytes.saturating_sub(want_stdout));
    let remaining = max_bytes.saturating_sub(want_stdout + stderr_take);
    let stdout_take = want_stdout + remaining.min(stdout.text.len().saturating_sub(want_stdout));

    aggregated.extend_from_slice(&stdout.text[..stdout_take]);
    aggregated.extend_from_slice(&stderr.text[..stderr_take]);

    StreamOutput {
        text: aggregated,
        truncated_after_lines: None,
    }
}

TESTS

  • just fmt
  • just fix -p codex-core
  • cargo test -p codex-core aggregate_output_
  • cargo test -p codex-core
  • cargo test --all-features

FIXES

Fixes #9758

@Kbediako

Copy link
Copy Markdown
Contributor Author

Note: 6 "Local Bazel build" checks are failing. The logs show BuildBuddy cache auth errors (UNAUTHENTICATED: missing API key), which is typical for forked PRs; not a code failure on this change.

Comment thread codex-rs/core/src/exec.rs Outdated
@etraut-openai etraut-openai added the needs-response Additional information is requested label Jan 24, 2026
@Kbediako Kbediako requested a review from jif-oai January 26, 2026 14:26
@etraut-openai etraut-openai removed the needs-response Additional information is requested label Jan 26, 2026
Comment thread codex-cli/tests/signal-forwarding.test.mjs Outdated
Comment thread codex-cli/bin/codex.js
Comment thread codex-rs/core/src/exec.rs Outdated
Comment thread codex-rs/core/src/exec.rs Outdated
@Kbediako Kbediako requested a review from jif-oai January 27, 2026 05:22
@jif-oai

jif-oai commented Jan 27, 2026

Copy link
Copy Markdown
Collaborator

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown
Contributor

Codex Review: Didn't find any major issues. Can't wait for the next one!

ℹ️ About Codex in GitHub

Your team has set up Codex to 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 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@jif-oai jif-oai left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

lgtm

Comment thread codex-rs/core/src/exec.rs
let remaining = max_bytes.saturating_sub(want_stdout + stderr_take);
let stdout_take = want_stdout + remaining.min(stdout.text.len().saturating_sub(want_stdout));

aggregated.extend_from_slice(&stdout.text[..stdout_take]);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Just FYI: ok for this because in bytes but using [] accessor in Rust in generally dangerous as it can panic. For example if you have a string containing multi-bytes characters, this accessor can split a char between its bytes and it will panic the runtime

@jif-oai jif-oai merged commit ab99df0 into openai:main Jan 27, 2026
26 of 32 checks passed
@github-actions github-actions Bot locked and limited conversation to collaborators Jan 27, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug: exec aggregated output can exceed EXEC_OUTPUT_MAX_BYTES on async exec path

3 participants