Repository navigation
[codex] fix runtime cancellation state sync - #1638
Conversation
📝 WalkthroughWalkthroughAdds end-to-end cooperative cancellation for runtime tasks: the executor gains process-group management and oneshot cancel channels for Codex turns; the frontend message reducer gains an ChangesRuntime Task Cancellation
Sequence DiagramsequenceDiagram
participant UI as ChatInput / TaskForkDialog
participant PaneSession as useWorkbenchPaneSession
participant RuntimeMsg as useWorkbenchRuntimeMessaging
participant Executor as RuntimeWorkRpcHandler
participant Codex as run_codex_app_server_turn_with_cancel
participant OS as Unix Process Group
UI->>PaneSession: pauseCurrentResponse()
PaneSession->>RuntimeMsg: cancelRuntimePaneTask()
RuntimeMsg->>Executor: runtime.tasks.cancel
Executor->>Executor: abort_active_turn — send cancel_tx
Executor->>Codex: oneshot cancel fires
Codex->>Codex: tokio::select! cancellation wins
Codex->>OS: SIGTERM → SIGKILL to process group
Codex-->>Executor: Err(CODEX_APP_SERVER_TURN_CANCELLED)
Executor-->>RuntimeMsg: accepted: true
RuntimeMsg->>RuntimeMsg: refreshWorkLists()
RuntimeMsg-->>PaneSession: success
PaneSession->>PaneSession: dispatch assistant_cancelled
PaneSession-->>UI: streaming messages marked done/cancelled
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…ate-sync # Conflicts: # executor/tests/app_runtime_work_send_contract.rs
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
executor/src/runtime_work/handler.rs (1)
686-706: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winStore is marked
cancelledbefore the turn is confirmed stopped.
update_tasksetsstatus = "cancelled",running = false(Lines 691-695) beforeabort_active_turnis awaited. On thecancel_timeoutpath the turn is still alive and will later runfinish_local_task, overwriting status again — so the persisted state transiently disagrees with the live turn. Consider deferring the store mutation until afterabort_active_turnsucceeds, or documenting thatcancel_timeoutleaves a non-converged record.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@executor/src/runtime_work/handler.rs` around lines 686 - 706, The cancel flow in cancel_task updates the persisted task state to cancelled before abort_active_turn confirms the turn has actually stopped, which can leave the store out of sync on the cancel_timeout path. Move the store mutation in cancel_task so it only happens after abort_active_turn succeeds, or otherwise make the intended non-converged behavior explicit in the task record handling. Keep the localTaskId lookup and the abort_active_turn path as the key decision points when adjusting the logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@executor/src/agents/codex.rs`:
- Around line 502-517: In signal_codex_app_server_child, SIGTERM and SIGKILL are
being sent immediately one after the other, so the process group has no time to
exit cleanly. Update the Unix path to send SIGTERM first, then wait briefly or
poll try_wait on the Child before escalating to libc::SIGKILL, keeping the same
process-group targeting logic around child.id().
In `@executor/tests/app_runtime_work_send_contract.rs`:
- Around line 868-908: The helper write_fake_codex_traps_termination does not
actually trap TERM, so the runtime_tasks_cancel_terminates_running_codex_turn
test is not exercising the intended SIGKILL-after-trapped-SIGTERM behavior.
Update the shell script generated in write_fake_codex_traps_termination to
install a TERM trap similar to write_fake_codex_spawns_worker, while keeping the
existing logging and turn/start blocking behavior intact, so the fake Codex
process survives SIGTERM until the test sends SIGKILL.
---
Outside diff comments:
In `@executor/src/runtime_work/handler.rs`:
- Around line 686-706: The cancel flow in cancel_task updates the persisted task
state to cancelled before abort_active_turn confirms the turn has actually
stopped, which can leave the store out of sync on the cancel_timeout path. Move
the store mutation in cancel_task so it only happens after abort_active_turn
succeeds, or otherwise make the intended non-converged behavior explicit in the
task record handling. Keep the localTaskId lookup and the abort_active_turn path
as the key decision points when adjusting the logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d92f1a9d-2260-4a83-b536-24d998ec7cf9
📒 Files selected for processing (15)
docs/en/developer-guide/task-runtime-state-machine.mddocs/zh/developer-guide/task-runtime-state-machine.mdexecutor/src/agents/codex.rsexecutor/src/agents/mod.rsexecutor/src/runtime_work/handler.rsexecutor/src/runtime_work/store.rsexecutor/tests/app_runtime_work_contract.rsexecutor/tests/app_runtime_work_send_contract.rspackages/chat-core/src/workbench-message-reducer.test.tspackages/chat-core/src/workbench-message-reducer.tswework/src/components/layout/DesktopWorkbenchMain.tsxwework/src/components/layout/MobileWorkbenchLayout.tsxwework/src/components/layout/useWorkbenchPaneSession.tswework/src/features/workbench/WorkbenchProvider.test.tsxwework/src/features/workbench/useWorkbenchRuntimeMessaging.ts
| fn signal_codex_app_server_child(child: &mut Child) { | ||
| if matches!(child.try_wait(), Ok(Some(_))) { | ||
| return; | ||
| } | ||
|
|
||
| #[cfg(unix)] | ||
| if let Some(process_group_id) = child.id() { | ||
| unsafe { | ||
| let _ = libc::kill(-(process_group_id as libc::pid_t), libc::SIGTERM); | ||
| let _ = libc::kill(-(process_group_id as libc::pid_t), libc::SIGKILL); | ||
| } | ||
| return; | ||
| } | ||
|
|
||
| let _ = child.start_kill(); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
libc setpgid pre_exec child process group kill negative pid ESRCH tokio
💡 Result:
The ESRCH error in the context of sending signals to process groups or setting process group IDs generally stems from a mismatch between the expected PID/PGID and the actual state of the process table [1][2][3]. Killing a Process Group with a Negative PID: When using the kill system call with a negative PID (e.g., kill(-pgid, sig)), the kernel attempts to send the signal to all processes in the process group whose ID equals the absolute value of that PID [4]. An ESRCH error occurs if no process group exists with that ID [4][3]. A common mistake is assuming that a PID is always a valid PGID; however, only the process group leader has a PGID equal to its PID [5][3]. If you wish to signal an entire group, you must ensure you are passing the correct PGID, which can be retrieved via getpgid(pid) [3]. setpgid in pre_exec: When using pre_exec (often in Rust's tokio or std::process) to set the process group of a child process, the setpgid call must occur after fork but before the child executes a new program via execve [6][7]. The setpgid call itself may return ESRCH if the specified process is not the calling process or its child, or if the process has already executed an exec function (which typically triggers EACCES, though behavior with zombies or race conditions can sometimes lead to ESRCH) [1][2][8]. Tokio and Process Groups: To manage process groups in Tokio, the recommended approach is to use the.process_group(pgid) method on tokio::process::Command [9]. This method is stable and allows you to set the PGID for the child process without manually managing pre_exec or unsafe blocks, avoiding potential errors associated with incorrect timing or invalid PID/PGID assumptions [6][9][10]. If you must use pre_exec for custom logic, ensure that you only call setpgid on the child process itself (using getpid inside the child) or its direct children, and handle potential errors gracefully [6].
Citations:
- 1: https://man7.org/linux/man-pages/man2/setpgid.2.html
- 2: https://www.man7.org/linux/man-pages/man3/setpgid.3p.html
- 3: https://stackoverflow.com/questions/55561984/how-to-fix-no-such-process-error-from-kill-function-on-negation-of-pid
- 4: https://docs.oracle.com/cd/E36784_01/html/E36872/kill-2.html
- 5: https://unix.stackexchange.com/questions/68754/kill-a-group-of-processes-with-negative-pid
- 6: Create a tokio::process::Child in a new process group tokio-rs/tokio#4312
- 7: https://linux.die.net/man/3/setpgid
- 8: https://stackoverflow.com/questions/48545762/setpgid-for-child-zombie-process-gives-esrch-instead-of-eacces
- 9: https://github.com/tokio-rs/tokio/blob/306ed1c3/tokio/src/process/mod.rs
- 10: https://rust-lang.github.io/rfcs/3228-process-process_group.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== codex.rs outline ==\n'
ast-grep outline executor/src/agents/codex.rs --view expanded || true
printf '\n== Relevant slices ==\n'
sed -n '380,540p' executor/src/agents/codex.rs | cat -n
printf '\n== Search for process group setup / termination paths ==\n'
rg -n "setpgid|process_group|start_kill\(|wait\(|try_wait\(|kill\(-" executor/src/agents/codex.rs executor/src -g '!**/target/**' || trueRepository: wecode-ai/Wegent
Length of output: 18312
Add a short grace window before escalating to SIGKILL. SIGTERM and SIGKILL are sent back-to-back here, so the child never gets a chance to shut down cleanly. Wait briefly or poll for exit before killing the group.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@executor/src/agents/codex.rs` around lines 502 - 517, In
signal_codex_app_server_child, SIGTERM and SIGKILL are being sent immediately
one after the other, so the process group has no time to exit cleanly. Update
the Unix path to send SIGTERM first, then wait briefly or poll try_wait on the
Child before escalating to libc::SIGKILL, keeping the same process-group
targeting logic around child.id().
| fn write_fake_codex_traps_termination(log_path: &Path) -> PathBuf { | ||
| let path = temp_path("fake-codex-cancel-kill", "sh"); | ||
| let _ = fs::remove_file(log_path); | ||
| let content = format!( | ||
| r#"#!/bin/sh | ||
| LOG_PATH='{}' | ||
| case "$*" in | ||
| *' -c '*|'-c '*) | ||
| printf 'turn-pid:%s\n' "$$" >> "$LOG_PATH" | ||
| ;; | ||
| *) | ||
| printf 'persistent-pid:%s\n' "$$" >> "$LOG_PATH" | ||
| ;; | ||
| esac | ||
| while IFS= read -r line; do | ||
| printf '%s\n' "$line" >> "$LOG_PATH" | ||
| request_id=$(printf '%s\n' "$line" | sed -n 's/.*"id":\([0-9][0-9]*\).*/\1/p') | ||
| case "$line" in | ||
| *'"method":"initialize"'*) | ||
| printf '%s\n' '{{"id":'"$request_id"',"result":{{"protocolVersion":1}}}}' | ||
| ;; | ||
| *'"method":"initialized"'*) | ||
| ;; | ||
| *'"method":"thread/start"'*) | ||
| printf '%s\n' '{{"id":'"$request_id"',"result":{{"thread":{{"id":"thread-1"}}}}}}' | ||
| ;; | ||
| *'"method":"thread/name/set"'*) | ||
| printf '%s\n' '{{"id":'"$request_id"',"result":{{}}}}' | ||
| ;; | ||
| *'"method":"turn/start"'*) | ||
| printf '%s\n' '{{"id":'"$request_id"',"result":{{"turn":{{"id":"turn-1","status":"inProgress"}}}}}}' | ||
| while true; do sleep 1; done | ||
| ;; | ||
| esac | ||
| done | ||
| "#, | ||
| log_path.display() | ||
| ); | ||
| write_executable(&path, &content); | ||
| path | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
write_fake_codex_traps_termination never installs a signal trap.
The helper name and the companion test (runtime_tasks_cancel_terminates_running_codex_turn) imply the fake process traps termination so the test proves SIGKILL still wins. But this script has no trap command, so a plain SIGTERM already terminates it — the SIGKILL-vs-trapped-SIGTERM scenario isn't actually exercised. Compare with write_fake_codex_spawns_worker (Line 938), which correctly uses trap "" TERM.
🧪 Add a TERM trap to match the intent
LOG_PATH='{}'
+trap "" TERM
case "$*" in📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fn write_fake_codex_traps_termination(log_path: &Path) -> PathBuf { | |
| let path = temp_path("fake-codex-cancel-kill", "sh"); | |
| let _ = fs::remove_file(log_path); | |
| let content = format!( | |
| r#"#!/bin/sh | |
| LOG_PATH='{}' | |
| case "$*" in | |
| *' -c '*|'-c '*) | |
| printf 'turn-pid:%s\n' "$$" >> "$LOG_PATH" | |
| ;; | |
| *) | |
| printf 'persistent-pid:%s\n' "$$" >> "$LOG_PATH" | |
| ;; | |
| esac | |
| while IFS= read -r line; do | |
| printf '%s\n' "$line" >> "$LOG_PATH" | |
| request_id=$(printf '%s\n' "$line" | sed -n 's/.*"id":\([0-9][0-9]*\).*/\1/p') | |
| case "$line" in | |
| *'"method":"initialize"'*) | |
| printf '%s\n' '{{"id":'"$request_id"',"result":{{"protocolVersion":1}}}}' | |
| ;; | |
| *'"method":"initialized"'*) | |
| ;; | |
| *'"method":"thread/start"'*) | |
| printf '%s\n' '{{"id":'"$request_id"',"result":{{"thread":{{"id":"thread-1"}}}}}}' | |
| ;; | |
| *'"method":"thread/name/set"'*) | |
| printf '%s\n' '{{"id":'"$request_id"',"result":{{}}}}' | |
| ;; | |
| *'"method":"turn/start"'*) | |
| printf '%s\n' '{{"id":'"$request_id"',"result":{{"turn":{{"id":"turn-1","status":"inProgress"}}}}}}' | |
| while true; do sleep 1; done | |
| ;; | |
| esac | |
| done | |
| "#, | |
| log_path.display() | |
| ); | |
| write_executable(&path, &content); | |
| path | |
| } | |
| fn write_fake_codex_traps_termination(log_path: &Path) -> PathBuf { | |
| let path = temp_path("fake-codex-cancel-kill", "sh"); | |
| let _ = fs::remove_file(log_path); | |
| let content = format!( | |
| r#"#!/bin/sh | |
| LOG_PATH='{}' | |
| trap "" TERM | |
| case "$*" in | |
| *' -c '*|'-c '*) | |
| printf 'turn-pid:%s\n' "$$" >> "$LOG_PATH" | |
| ;; | |
| *) | |
| printf 'persistent-pid:%s\n' "$$" >> "$LOG_PATH" | |
| ;; | |
| esac | |
| while IFS= read -r line; do | |
| printf '%s\n' "$line" >> "$LOG_PATH" | |
| request_id=$(printf '%s\n' "$line" | sed -n 's/.*"id":\([0-9][0-9]*\).*/\1/p') | |
| case "$line" in | |
| *'"method":"initialize"'*) | |
| printf '%s\n' '{{"id":'"$request_id"',"result":{{"protocolVersion":1}}}}' | |
| ;; | |
| *'"method":"initialized"'*) | |
| ;; | |
| *'"method":"thread/start"'*) | |
| printf '%s\n' '{{"id":'"$request_id"',"result":{{"thread":{{"id":"thread-1"}}}}}}' | |
| ;; | |
| *'"method":"thread/name/set"'*) | |
| printf '%s\n' '{{"id":'"$request_id"',"result":{{}}}}' | |
| ;; | |
| *'"method":"turn/start"'*) | |
| printf '%s\n' '{{"id":'"$request_id"',"result":{{"turn":{{"id":"turn-1","status":"inProgress"}}}}}}' | |
| while true; do sleep 1; done | |
| ;; | |
| esac | |
| done | |
| "#, | |
| log_path.display() | |
| ); | |
| write_executable(&path, &content); | |
| path | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@executor/tests/app_runtime_work_send_contract.rs` around lines 868 - 908, The
helper write_fake_codex_traps_termination does not actually trap TERM, so the
runtime_tasks_cancel_terminates_running_codex_turn test is not exercising the
intended SIGKILL-after-trapped-SIGTERM behavior. Update the shell script
generated in write_fake_codex_traps_termination to install a TERM trap similar
to write_fake_codex_spawns_worker, while keeping the existing logging and
turn/start blocking behavior intact, so the fake Codex process survives SIGTERM
until the test sends SIGKILL.
1 participant
Summary
cancel_timeoutinstead of treating a live turn as stopped.assistant_cancelled.Root Cause
Stopping previously could halt the stream or parent process while the underlying Codex app-server turn, or its child worker processes, continued running. On the frontend, runtime work state and pane message streaming state were also updated through separate paths, so one UI surface could show stopped while another still showed running.
Verification
./node_modules/.bin/vitest run src/features/workbench/WorkbenchProvider.test.tsx../../node_modules/.bin/vitest run src/workbench-message-reducer.test.ts../../node_modules/.bin/tsc -p tsconfig.json --noEmitcargo fmt && uv run cargo test --test app_runtime_work_send_contract --test codex_app_server_contractcargo fmt --check,cargo test --all-features,cargo clippySummary by CodeRabbit
New Features
Bug Fixes
Tests