mirror of
https://github.com/openai/codex.git
synced 2026-10-28 18:58:46 +00:00
Fix code mode yield startup race
Co-authored-by: Codex <noreply@openai.com>
This commit is contained in:
@@ -78,6 +78,9 @@ Older failures also appeared on Linux, but the repeated cross-PR signal is stron
|
||||
- The next follow-up keeps the `cmd.exe` harness but switches the nested read to a UTF-8 `-EncodedCommand` script, removing the extra quoting layer without changing the test's product coverage.
|
||||
- Run `23176240418` on head `6a302cb2e` proved the quoting fix was only partial: both Windows `Tests` shards still failed in `all::suite::apply_patch_cli::apply_patch_cli_can_use_shell_command_output_as_patch_input`, and check-run annotations `67338924441` and `67338924464` showed the nested `powershell.exe` invocation was appending `CLIXML` progress records (`Preparing modules for first use.`) after the expected file contents.
|
||||
- Current follow-up: keep the `cmd.exe` harness and UTF-8 encoded nested PowerShell read, but add `$ProgressPreference = 'SilentlyContinue'`, switch the read to `[System.IO.File]::ReadAllText(...)`, and add `-NonInteractive` so the shell tool returns only the source bytes instead of PowerShell progress serialization noise.
|
||||
- Run `23177624353` on doc-only head `65f7f64ae` failed only in `Tests — ubuntu-24.04 - x86_64-unknown-linux-gnu`, in `codex-core::all::suite::code_mode::code_mode_yield_timeout_works_for_busy_loop`. The only diff from the prior green head `4a3181290` was one extra line in this tracking document, so the failure exposed a latent flake rather than a branch regression.
|
||||
- CI annotation `67343276143` showed the busy-loop test yielded only the synthetic `Script running with cell ID ...` header and missed the expected `phase 1` output item. The root cause is in `core/src/tools/code_mode/runner.cjs`: the initial `yield_time_ms` timer started when the worker was created, so on a loaded runner the `100ms` budget could expire before the worker had begun executing `text("phase 1")`.
|
||||
- Current follow-up: make the worker emit an internal `started` message and arm the initial yield timer only after that signal arrives, so `yield_time_ms` measures script execution time instead of worker startup delay.
|
||||
- Rationale: these failures are test-harness flakes, not product behaviors. The fixes keep the assertions intact and remove environment-sensitive startup and ordering hazards instead of stretching timeouts.
|
||||
|
||||
## Constraints
|
||||
@@ -121,3 +124,4 @@ Older failures also appeared on Linux, but the repeated cross-PR signal is stron
|
||||
| `c14493260` | Fix Windows PowerShell parser stdin | failed | Run `23173027099` cleared the stale lint and `codex-shell-command` parser regressions, and `Bazel (experimental)` run `23173027042` finished green. The only remaining failures were `Tests — windows-x64 - x86_64-pc-windows-msvc` and `Tests — windows-arm64 - aarch64-pc-windows-msvc`, both in `all::suite::apply_patch_cli::apply_patch_cli_can_use_shell_command_output_as_patch_input`. CI annotations showed the `cmd.exe`-wrapped nested `powershell.exe -Command "Get-Content ..."` invocation printed the command string literal instead of reading the file, so the next follow-up replaces that nested command with a UTF-8 `-EncodedCommand` script. |
|
||||
| `6a302cb2e` | Encode Windows apply_patch read command | failed | Run `23176240418` kept all non-Windows jobs and `Bazel (experimental)` green, but both Windows `Tests` shards still failed in `all::suite::apply_patch_cli::apply_patch_cli_can_use_shell_command_output_as_patch_input`. Check-run annotations `67338924441` and `67338924464` showed the nested `powershell.exe` read was now executing, but it appended `CLIXML` progress output (`Preparing modules for first use.`) after the expected file contents. The next follow-up suppresses PowerShell progress records, uses `[System.IO.File]::ReadAllText(...)`, and adds `-NonInteractive` so the shell tool output stays deterministic. |
|
||||
| `4a3181290` | Suppress PowerShell CLIXML in apply_patch test | full pass | Runs `23176985939` (`rust-ci`) and `23176985980` (`Bazel (experimental)`) finished green for the full PR, including both Windows `Tests` jobs. The Windows `cmd.exe` harness now keeps the nested UTF-8 PowerShell read but suppresses progress records, switches to `[System.IO.File]::ReadAllText(...)`, and adds `-NonInteractive` so the shell tool returns only the source bytes. This is pass 1 of 5 after the latest failing run. |
|
||||
| `65f7f64ae` | Record validation pass 1 | failed | Run `23177624353` failed only in `Tests — ubuntu-24.04 - x86_64-unknown-linux-gnu`, in `codex-core::all::suite::code_mode::code_mode_yield_timeout_works_for_busy_loop`, while `Bazel (experimental)` run `23177624352` finished green on the same head. The only diff from the prior full-green head `4a3181290` was one extra line in this tracking document, so the failure exposed a latent code-mode yield race: the runner started the initial `yield_time_ms` timer when the worker was created, allowing the `100ms` budget to expire before the worker began executing user code on a loaded CI machine. |
|
||||
|
||||
@@ -465,6 +465,7 @@ function codeModeWorkerMain() {
|
||||
writable: false,
|
||||
});
|
||||
|
||||
parentPort.postMessage({ type: 'started' });
|
||||
try {
|
||||
await runModule(context, start, callTool, helpers);
|
||||
parentPort.postMessage({
|
||||
@@ -639,6 +640,10 @@ function startSession(protocol, sessions, start) {
|
||||
content_items: [],
|
||||
default_yield_time_ms: normalizeYieldTime(start.default_yield_time_ms),
|
||||
id: start.cell_id,
|
||||
initial_yield_time_ms:
|
||||
start.yield_time_ms == null
|
||||
? normalizeYieldTime(start.default_yield_time_ms)
|
||||
: normalizeYieldTime(start.yield_time_ms),
|
||||
initial_yield_timer: null,
|
||||
initial_yield_triggered: false,
|
||||
max_output_tokens_per_exec_call: maxOutputTokensPerExecCall,
|
||||
@@ -651,11 +656,6 @@ function startSession(protocol, sessions, start) {
|
||||
}),
|
||||
};
|
||||
sessions.set(session.id, session);
|
||||
const initialYieldTime =
|
||||
start.yield_time_ms == null
|
||||
? session.default_yield_time_ms
|
||||
: normalizeYieldTime(start.yield_time_ms);
|
||||
scheduleInitialYield(protocol, session, initialYieldTime);
|
||||
|
||||
session.worker.on('message', (message) => {
|
||||
void handleWorkerMessage(protocol, sessions, session, message).catch((error) => {
|
||||
@@ -694,6 +694,11 @@ async function handleWorkerMessage(protocol, sessions, session, message) {
|
||||
return;
|
||||
}
|
||||
|
||||
if (message.type === 'started') {
|
||||
scheduleInitialYield(protocol, session, session.initial_yield_time_ms);
|
||||
return;
|
||||
}
|
||||
|
||||
if (message.type === 'yield') {
|
||||
void sendYielded(protocol, session);
|
||||
return;
|
||||
|
||||
Reference in New Issue
Block a user