[js_repl] Hard-stop active js_repl execs on explicit user interrupts#13329
[js_repl] Hard-stop active js_repl execs on explicit user interrupts#13329aaronl-openai merged 9 commits intomainfrom
Conversation
aaronl-openai
changed the title
|
@codex review |
|
Codex Review: Didn't find any major issues. Chef's kiss. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
my local codex claimed this was an issue, not sure why it wasn't caught by the review bot. can you take a look? [P2] mod.rs (line 714) still misses one interrupt race. execute() starts/stashes the kernel earlier at mod.rs (line 749) and only registers/marks the active exec later at mod.rs (line 794). If Ctrl-C lands in that gap, handle_task_abort() calls interrupt_turn_exec(), it sees submitted == false, returns false, and the turn can leave behind a live Node kernel. I’d ask for that window to be cleaned up before approving. |
Track when a turn has started a fresh js_repl kernel for the current exec attempt so interrupt cleanup can reset it before any exec is formally submitted. This closes the remaining startup-window race where an explicit stop could land after the kernel was created but before current exec state was visible, leaving a live Node kernel behind. Add regression coverage for interrupting that pending-kernel-start state. Co-authored-by: Codex <noreply@openai.com>
codex-rs/core/src/tasks/mod.rs
Outdated
| .abort(session_ctx, Arc::clone(&task.turn_context)) | ||
| .await; | ||
|
|
||
| if reason == TurnAbortReason::Interrupted |
There was a problem hiding this comment.
Can we avoid making Session::handle_task_abort() know about js_repl specifically? I agree we need explicit interrupt cleanup for long-lived runtimes, but this feels like orchestration code accumulating tool-specific teardown. Since we already have a separate interrupt-cleanup path for unified_exec, could we push this behind something like a more generic turn/session-level cleanup_after_interrupt() hook instead, so tasks/mod.rs stays responsible for abort flow rather than individual tool/runtime details?
Move interrupt cleanup behind a Session-level hook and collapse js_repl interrupt tracking into a single top-level exec state machine. Co-authored-by: Codex <noreply@openai.com>
fjord-oai
left a comment
fjord-oai
left a comment
There was a problem hiding this comment.
approved, but get codex team input on the new interrupt cleanup mechanism
…terrupt # Conflicts: # codex-rs/core/src/tools/js_repl/mod.rs
| .await; | ||
| } | ||
|
|
||
| pub(crate) async fn cleanup_after_interrupt(&self, turn_context: &Arc<TurnContext>) { |
There was a problem hiding this comment.
interesting, why break this out into a separate file? don't see a lot of value in doing so. could this just be methods in tasks/mod.rs?
…x into dev/aaronl/js-repl-interrupt
3 participants
Summary
js_replonly forTurnAbortReason::Interrupted, preserving the persistent REPL across replaced turnsWhy
Stopping a turn previously surfaced
aborted by user after Xseven though the underlyingjs_replkernel could continue executing. Earlier fixes also risked resetting the session-scoped REPL too broadly or missing already-dispatched work. This change keeps cleanup scoped to explicit stop semantics and makes the interrupt path line up with both submitted execs and newly started kernels.Testing
just fmtcargo test -p codex-corejust fix -p codex-corecargo test -p codex-corepasses the updatedjs_replcoverage, including the new startup-window regression test, but still has unrelated integration failures in this environment outsidejs_repl.