From c2617273706f142ea1cc7196a2d423893b0767e4 Mon Sep 17 00:00:00 2001 From: cassirer-openai Date: Fri, 9 Oct 2026 19:41:34 +0000 Subject: [PATCH] Preserve code mode cancellation during output serialization (#52685) ## Why Reentering V8 during termination can replace uncatchable cancellation with a catchable JavaScript exception, allowing a script to continue after cancellation. ## What changed Skip JSON conversion and exception throwing while V8 execution is terminating, preserving cancellation through `image()` getters and JSON fallback. ## Testing Add regression coverage for cancellation during JSON fallback and reentrant image getters, including prompt termination through the standalone host and continued usability of another session. Extend serialization error tests to cover failing image getters alongside circular JSON. GitOrigin-RevId: f03dd131a62724106fad96111292ddf5b9dc99f6 --- codex-rs/code-mode-host/tests/grpc.rs | 2 + .../tests/grpc/image_cancellation_tests.rs | 98 +++++++++++++++++++ codex-rs/code-mode-runtime/src/runtime/mod.rs | 84 ++++++++++------ .../code-mode-runtime/src/runtime/value.rs | 8 ++ .../code-mode-runtime/src/service_tests.rs | 82 +++++++++------- 5 files changed, 207 insertions(+), 67 deletions(-) create mode 100644 codex-rs/code-mode-host/tests/grpc/image_cancellation_tests.rs diff --git a/codex-rs/code-mode-host/tests/grpc.rs b/codex-rs/code-mode-host/tests/grpc.rs index 8f584a2203..aa44f19510 100644 --- a/codex-rs/code-mode-host/tests/grpc.rs +++ b/codex-rs/code-mode-host/tests/grpc.rs @@ -46,6 +46,8 @@ use tonic::transport::Server; mod admission_tests; #[path = "support/host.rs"] mod host; +#[path = "grpc/image_cancellation_tests.rs"] +mod image_cancellation_tests; #[path = "support/large_tool_delegate.rs"] mod large_tool_delegate; #[path = "grpc/network_policy_tests.rs"] diff --git a/codex-rs/code-mode-host/tests/grpc/image_cancellation_tests.rs b/codex-rs/code-mode-host/tests/grpc/image_cancellation_tests.rs new file mode 100644 index 0000000000..28a160a466 --- /dev/null +++ b/codex-rs/code-mode-host/tests/grpc/image_cancellation_tests.rs @@ -0,0 +1,98 @@ +//! Cancellation of reentrant image getters through the standalone host. + +use super::*; +use pretty_assertions::assert_eq; + +#[tokio::test] +async fn cancellation_stops_reentrant_image_getter() -> Result<()> { + // Force reentry and JSON fallback at a shallow depth, independent of stack limits. + let source = r#" +let calls = 0; +const deadline = Date.now() + 60000; +const value = { + get image_url() { + switch (++calls) { + case 1: image(value); return ""; + case 2: throw new Error("enter fallback"); + default: + notify("started"); + while (Date.now() < deadline) {} + return ""; + } + } +}; +try { image(value); } catch { text(calls); } +"#; + let host = HostHarness::start("grpc://127.0.0.1:0").await?; + let provider = GrpcCodeModeSessionProvider::new(host.endpoint); + let session = provider + .create_session() + .await + .map_err(anyhow::Error::msg)?; + let other_session = provider + .create_session() + .await + .map_err(anyhow::Error::msg)?; + let other_response = execute( + &other_session, + request(r#"store("alive", "yes"); text("yes");"#), + Arc::new(NoopCodeModeSessionDelegate), + ) + .await?; + assert_eq!( + other_response, + text_response("1", "yes", other_response.code_mode_host_duration()) + ); + + let delegate = Arc::new(RecordingDelegate::default()); + let mut pending = request(source); + pending.yield_time_ms = Some(60_000); + let started = session + .execute(pending, delegate.clone(), /*preempt*/ None) + .await + .map_err(anyhow::Error::msg)?; + let id = started.cell_id.clone(); + // The second image call enters JSON fallback; wait until its getter is running. + timeout(TEST_TIMEOUT, delegate.notification_delivered.notified()) + .await + .context("getter did not signal startup")?; + + let result = timeout(Duration::from_secs(1), session.terminate(id.clone())) + .await + .context("workload did not stop within the cancellation deadline")? + .map_err(anyhow::Error::msg)?; + // The getter would otherwise run for 60 seconds and emit its counter. + assert_eq!( + result, + WaitOutcome::LiveCell(RuntimeResponse::Terminated { + code_mode_host_duration: result.code_mode_host_duration(), + cell_id: id.clone(), + content_items: Vec::new(), + }) + ); + let initial = timeout(TEST_TIMEOUT, started.initial_response()) + .await + .context("initial termination response did not arrive")? + .map_err(anyhow::Error::msg)?; + assert_eq!( + initial, + RuntimeResponse::Terminated { + code_mode_host_duration: initial.code_mode_host_duration(), + cell_id: id, + content_items: Vec::new(), + } + ); + let other_response = execute( + &other_session, + request(r#"text(load("alive"));"#), + Arc::new(NoopCodeModeSessionDelegate), + ) + .await?; + assert_eq!( + other_response, + text_response("2", "yes", other_response.code_mode_host_duration()) + ); + session.shutdown().await.map_err(anyhow::Error::msg)?; + other_session.shutdown().await.map_err(anyhow::Error::msg)?; + Ok(()) +} diff --git a/codex-rs/code-mode-runtime/src/runtime/mod.rs b/codex-rs/code-mode-runtime/src/runtime/mod.rs index ae587c0233..7a3904fbce 100644 --- a/codex-rs/code-mode-runtime/src/runtime/mod.rs +++ b/codex-rs/code-mode-runtime/src/runtime/mod.rs @@ -429,40 +429,64 @@ mod tests { } #[tokio::test] - async fn terminate_execution_stops_cpu_bound_module() { - let (event_tx, mut event_rx) = mpsc::unbounded_channel(); - let (_runtime_tx, _runtime_control_tx, runtime_terminate_handle) = spawn_runtime( - HashMap::new(), - execute_request("while (true) {}"), - event_tx, - PendingRuntimeMode::Continue, - /*task_failure_handler*/ None, - ) - .unwrap(); - - let started_event = tokio::time::timeout(Duration::from_secs(1), event_rx.recv()) - .await - .unwrap() + async fn terminate_execution_stops_javascript() { + for source in [ + r#"notify("started"); while (true) {}"#, + // The failed first read enters JSON fallback, whose TryCatch must preserve termination. + r#" +let reads = 0; +const value = { + get image_url() { + if (++reads === 1) throw new Error("enter fallback"); + notify("started"); + while (true) {} + } +}; +try { image(value); } catch {} +text("continued after cancellation"); +"#, + ] { + let (event_tx, mut event_rx) = mpsc::unbounded_channel(); + let (_runtime_tx, _runtime_control_tx, runtime_terminate_handle) = spawn_runtime( + HashMap::new(), + execute_request(source), + event_tx, + PendingRuntimeMode::Continue, + /*task_failure_handler*/ None, + ) .unwrap(); - assert!(matches!(started_event, RuntimeEvent::Started)); - assert!(runtime_terminate_handle.terminate_execution()); - - let result_event = tokio::time::timeout(Duration::from_secs(1), event_rx.recv()) - .await - .unwrap() - .unwrap(); - let RuntimeEvent::Result { error_text, .. } = result_event else { - panic!("expected runtime result after termination"); - }; - assert!(error_text.is_some()); - - assert!( - tokio::time::timeout(Duration::from_secs(1), event_rx.recv()) + let started_event = tokio::time::timeout(Duration::from_secs(1), event_rx.recv()) .await .unwrap() - .is_none() - ); + .unwrap(); + assert!(matches!(started_event, RuntimeEvent::Started)); + + // Started precedes evaluation; the notification proves the workload is running. + assert!(matches!( + tokio::time::timeout(Duration::from_secs(1), event_rx.recv()) + .await + .unwrap(), + Some(RuntimeEvent::Notify { .. }) + )); + assert!(runtime_terminate_handle.terminate_execution()); + + let result_event = tokio::time::timeout(Duration::from_secs(1), event_rx.recv()) + .await + .unwrap() + .unwrap(); + let RuntimeEvent::Result { error_text, .. } = result_event else { + panic!("expected runtime result after termination"); + }; + assert!(error_text.is_some()); + + assert!( + tokio::time::timeout(Duration::from_secs(1), event_rx.recv()) + .await + .unwrap() + .is_none() + ); + } } #[tokio::test] diff --git a/codex-rs/code-mode-runtime/src/runtime/value.rs b/codex-rs/code-mode-runtime/src/runtime/value.rs index e5305a6979..13e49159e4 100644 --- a/codex-rs/code-mode-runtime/src/runtime/value.rs +++ b/codex-rs/code-mode-runtime/src/runtime/value.rs @@ -297,6 +297,10 @@ pub(super) fn v8_value_to_json( scope: &mut v8::PinScope<'_, '_>, value: v8::Local<'_, v8::Value>, ) -> Result, String> { + // Reentering V8 during termination can replace it with a catchable exception. + if scope.is_execution_terminating() { + return Err("JavaScript execution terminated".to_string()); + } // V8 stringifies undefined as the non-JSON text "undefined". if value.is_undefined() { return Ok(None); @@ -362,6 +366,10 @@ pub(super) fn value_to_error_text( } pub(super) fn throw_type_error(scope: &mut v8::PinScope<'_, '_>, message: &str) { + // Preserve V8's uncatchable termination instead of replacing it with a JS error. + if scope.is_execution_terminating() { + return; + } if let Some(message) = v8::String::new(scope, message) { scope.throw_exception(message.into()); } diff --git a/codex-rs/code-mode-runtime/src/service_tests.rs b/codex-rs/code-mode-runtime/src/service_tests.rs index 2f675249f4..138d6234d1 100644 --- a/codex-rs/code-mode-runtime/src/service_tests.rs +++ b/codex-rs/code-mode-runtime/src/service_tests.rs @@ -1445,45 +1445,53 @@ async fn text_helper_serializes_objects() { } #[tokio::test] -async fn text_helper_surfaces_stringify_errors() { - let service = InProcessCodeModeSession::new(); +async fn output_helpers_surface_serialization_errors() { + for (source, expected_error) in [ + ( + "const circular = {}; circular.self = circular; text(circular);", + "Converting circular structure to JSON", + ), + ( + "image({get image_url() { throw new Error('image getter failed'); }});", + "image getter failed", + ), + ( + "image({type: 'image', get data() { throw new Error('MCP getter failed'); }});", + "MCP getter failed", + ), + ] { + let service = InProcessCodeModeSession::new(); + let response = execute( + &service, + ExecuteRequest { + yield_time_ms: None, + ..execute_request(source) + }, + ) + .await; - let response = execute( - &service, - ExecuteRequest { - source: r#" -const circular = {}; -circular.self = circular; -text(circular); -"# - .to_string(), - yield_time_ms: None, - ..execute_request("") - }, - ) - .await; - - let RuntimeResponse::Result { - error_text: Some(error_text), - .. - } = &response - else { - panic!("circular stringify unexpectedly succeeded: {response:?}"); - }; - assert!( - error_text.contains("Converting circular structure to JSON"), - "unexpected circular stringify error: {error_text}" - ); - let error_text = error_text.clone(); - assert_eq!( - response, - RuntimeResponse::Result { - code_mode_host_duration: None, - cell_id: cell_id("1"), - content_items: Vec::new(), + let RuntimeResponse::Result { error_text: Some(error_text), - } - ); + .. + } = &response + else { + panic!("serialization unexpectedly succeeded: {response:?}"); + }; + assert!( + error_text.contains(expected_error), + "unexpected serialization error: {error_text}" + ); + let error_text = error_text.clone(); + assert_eq!( + response, + RuntimeResponse::Result { + code_mode_host_duration: None, + cell_id: cell_id("1"), + content_items: Vec::new(), + error_text: Some(error_text), + } + ); + } } #[tokio::test]