feat: waiting for an elicitation should not count against a shell tool timeout by bolinfest · Pull Request #6973 · openai/codex

bolinfest · 2025-11-20T07:07:58Z

Previously, we were running into an issue where we would run the shell tool call with a timeout of 10s, but it fired an elicitation asking for user approval, the time the user took to respond to the elicitation was counted agains the 10s timeout, so the shell tool call would fail with a timeout error unless the user is very fast!

This PR addresses this issue by introducing a "stopwatch" abstraction that is used to manage the timeout. The idea is:

Stopwatch::new() is called with the real timeout of the shell tool call.
process_exec_tool_call() is called with the Cancellation variant of ExecExpiration because it should not manage its own timeout in this case
the Stopwatch expiration is wired up to the cancel_rx passed to process_exec_tool_call()
when an elicitation for the shell tool call is received, the Stopwatch pauses
because it is possible for multiple elicitations to arrive concurrently, it keeps track of the number of "active pauses" and does not resume until that counter goes down to zero

I verified that I can test the MCP server using @modelcontextprotocol/inspector and specify git status as the command with a timeout of 500ms and that the elicitation pops up and I have all the time in the world to respond whereas previous to this PR, that would not have been possible.

Stack created with Sapling. Best reviewed with ReviewStack.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

ℹ️ 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".

chatgpt-codex-connector · 2025-11-20T07:16:58Z

+        let effective_timeout_ms = params.timeout_ms.unwrap_or(DEFAULT_TIMEOUT_MS);
+        let stopwatch = Stopwatch::new(Some(effective_timeout_ms));
+        let cancel_rx = stopwatch.cancellation_receiver();
+        let process_timeout_ms = if cancel_rx.is_some() {
+            Some(u64::MAX)


Avoid overflowing tokio timeout in stopwatch path

Shell requests now set process_timeout_ms to Some(u64::MAX) whenever a stopwatch is created (mcp.rs lines 94-98). That value flows into core/src/exec.rs where it becomes the Duration passed to tokio::time::timeout, and Tokio panics if the timeout exceeds its MAX_DURATION (≈584 years). u64::MAX ms (~584 billion years) will therefore panic the exec path before the cancellation receiver can fire, crashing the shell tool as soon as this path is exercised; use a bounded duration or skip the timeout instead of an overflowing value.

Useful? React with 👍 / 👎.

This is valid

Yep, fixed on the latest version. Now:

// We want a large timeout that is effectively infinite, because the // real timeout is enforced by the stopwatch and cancellation receiver. // While we could use u64::MAX here, using a more reasonable value // avoids potential issues with libraries that might not handle extreme // values well. let process_timeout_ms: u64 = 365 * 24 * 60 * 60 * 1000;

jif-oai · 2025-11-20T12:51:58Z

+        // While we could use u64::MAX here, using a more reasonable value
+        // avoids potential issues with libraries that might not handle extreme
+        // values well.
+        let process_timeout_ms: u64 = 365 * 24 * 60 * 60 * 1000;


I would not call this value "reasonable" regarding the scope of codex but fair enough :D

nornagon-openai · 2025-11-20T18:22:46Z

+        // We want a large timeout that is effectively infinite, because the
+        // real timeout is enforced by the stopwatch and cancellation receiver.
+        // While we could use u64::MAX here, using a more reasonable value
+        // avoids potential issues with libraries that might not handle extreme
+        // values well.
+        let process_timeout_ms: u64 = 365 * 24 * 60 * 60 * 1000;


why not use None for the timeout?

Because then it will default to 10s:

codex/codex-rs/core/src/exec.rs

Line 33 in a8a6cbd

const DEFAULT_TIMEOUT_MS: u64 = 10_000;

nornagon-openai · 2025-11-20T18:23:08Z

        timeout_ms: Option<u64>,
+        cancel_rx: Option<oneshot::Receiver<()>>,


It seems wrong to have both a timeout and a cancel token.

I guess I could try to make them exclusive in a follow-up?

Why not only have a cancel token? Leave it to the caller to set a timeout.

nornagon-openai · 2025-11-20T18:25:31Z

+                if remaining.is_zero() {
+                    break;
+                }


duplicative with elapsed >= limit above?

Great catch!

nornagon-openai · 2025-11-20T18:28:10Z

+    }
+
+    /// Runs `fut`, pausing the stopwatch while the future is pending. The clock resumes
+    /// automatically when the future completes. Nested calls are reference-counted so the


Suggested change

/// automatically when the future completes. Nested calls are reference-counted so the

/// automatically when the future completes. Nested/overlapping calls are reference-counted so the

…6972) This updates `ExecParams` so that instead of taking `timeout_ms: Option<u64>`, it now takes a more general cancellation mechanism, `ExecExpiration`, which is an enum that includes a `Cancellation(tokio_util::sync::CancellationToken)` variant. If the cancellation token is fired, then `process_exec_tool_call()` returns in the same way as if a timeout was exceeded. This is necessary so that in #6973, we can manage the timeout logic external to the `process_exec_tool_call()` because we want to "suspend" the timeout when an elicitation from a human user is pending. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/openai/codex/pull/6972). * #7005 * #6973 * __->__ #6972

…l timeout

bolinfest changed the base branch from main to pr6972 November 20, 2025 07:08

bolinfest mentioned this pull request Nov 20, 2025

feat: update process_exec_tool_call() to take a cancellation token #6972

Merged

chatgpt-codex-connector Bot reviewed Nov 20, 2025

View reviewed changes

bolinfest force-pushed the pr6973 branch 2 times, most recently from 4e9941d to b309236 Compare November 20, 2025 08:08

bolinfest requested a review from pakrym-oai November 20, 2025 08:19

bolinfest force-pushed the pr6973 branch from b309236 to 41d559f Compare November 20, 2025 08:20

bolinfest requested review from jif-oai and nornagon-openai November 20, 2025 08:21

bolinfest force-pushed the pr6973 branch from 41d559f to 905135c Compare November 20, 2025 08:28

jif-oai reviewed Nov 20, 2025

View reviewed changes

bolinfest mentioned this pull request Nov 20, 2025

feat: codex-shell-tool-mcp #7005

Merged

nornagon-openai reviewed Nov 20, 2025

View reviewed changes

bolinfest force-pushed the pr6973 branch from 905135c to 7cfd859 Compare November 20, 2025 21:50

bolinfest force-pushed the pr6972 branch from 9d420b4 to d41c652 Compare November 20, 2025 21:50

bolinfest force-pushed the pr6973 branch from 7cfd859 to a0e90b2 Compare November 20, 2025 23:05

bolinfest force-pushed the pr6972 branch 2 times, most recently from aec9655 to 208863f Compare November 20, 2025 23:23

bolinfest force-pushed the pr6973 branch from a0e90b2 to 782ed9a Compare November 20, 2025 23:23

bolinfest force-pushed the pr6972 branch from 208863f to e38be12 Compare November 20, 2025 23:44

bolinfest force-pushed the pr6973 branch from 782ed9a to 88c03b2 Compare November 20, 2025 23:44

bolinfest force-pushed the pr6972 branch from e38be12 to 4c158b0 Compare November 21, 2025 00:02

bolinfest force-pushed the pr6973 branch from 88c03b2 to 45586d7 Compare November 21, 2025 00:02

bolinfest force-pushed the pr6972 branch from 4c158b0 to 6e37a60 Compare November 21, 2025 00:07

bolinfest force-pushed the pr6973 branch 2 times, most recently from 0401532 to 50ab543 Compare November 21, 2025 00:14

bolinfest force-pushed the pr6972 branch from 6e37a60 to 8d3473b Compare November 21, 2025 00:16

bolinfest force-pushed the pr6973 branch 2 times, most recently from 24e97ab to 5f44599 Compare November 21, 2025 00:22

bolinfest requested a review from nornagon-openai November 21, 2025 00:25

bolinfest requested a review from jif-oai November 21, 2025 00:25

Base automatically changed from pr6972 to main November 21, 2025 00:29

feat: waiting for an elicitation should not count against a shell too…

f5ee3ee

…l timeout

bolinfest force-pushed the pr6973 branch from 5f44599 to f5ee3ee Compare November 21, 2025 00:30

nornagon-openai approved these changes Nov 21, 2025

View reviewed changes

bolinfest merged commit 8e5f38c into main Nov 21, 2025
47 of 50 checks passed

bolinfest deleted the pr6973 branch November 21, 2025 00:45

github-actions Bot locked and limited conversation to collaborators Nov 21, 2025

		timeout_ms: Option<u64>,
		cancel_rx: Option<oneshot::Receiver<()>>,

	/// automatically when the future completes. Nested calls are reference-counted so the
	/// automatically when the future completes. Nested/overlapping calls are reference-counted so the

Conversation

bolinfest commented Nov 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bolinfest commented Nov 20, 2025 •

edited

Loading