Skip to content

Conversation

@shijie-oai
Copy link
Collaborator

@shijie-oai shijie-oai commented Jan 9, 2026

Summary

  • Added mcpServer/refresh command to inform app servers and active threads to refresh mcpServer on next turn event.
  • Added pending_mcp_server_refresh_config to codex core so that if the value is populated, we reinitialize the mcp server manager on the thread level.
    • The config is updated on mcpServer/refresh command which we iterate through threads and provide with the latest config value after last write.

@shijie-oai shijie-oai force-pushed the shijie/hot-reload-mcp-servers branch from b0adb94 to 824fd95 Compare January 9, 2026 17:44
@shijie-oai shijie-oai force-pushed the shijie/hot-reload-mcp-servers branch from 824fd95 to 238497c Compare January 9, 2026 17:45
outgoing.send_response(request_id, response).await;
}

async fn mcp_server_refresh(&self, request_id: RequestId, _params: Option<()>) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not refresh the mcp connection manager for all active threads for inform them there is an update with the latest mcp config. It is up to the thread to refresh the connection on next active turn. This should help with performance and avoid excessive refresh if there is a lot of MCP changes at once.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe put this in a comment?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"It is up to the thread to refresh the connection on next active turn."

What does this mean?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All active threads in the thread manager would be aware of a new mcp config but we are not triggering an actual reinitialization yet. On a future turn (i.e. user submit a new prompt), if the thread detects a pending_mcp_config, then it would first reinitialize and then move onto the actual turn. The benefit here is that we are not doing any extra leg work - for example you started a thread and then we change the mcp servers but never reengage with that thread, then that thread would not refresh mcp server until called upon.


let thread_manager = Arc::clone(&self.thread_manager);
tokio::spawn(async move {
thread_manager.refresh_mcp_servers(refresh_config).await;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We build the mcp configs and pass it along to the thread so that they can have the latest slice on next refresh.

Comment on lines +1678 to +1688
let mut refreshed_manager = McpConnectionManager::default();
refreshed_manager
.initialize(
mcp_servers,
store_mode,
auth_statuses,
self.get_tx_event(),
cancel_token,
sandbox_state,
)
.await;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are doing a full reset here so that the mcp manager has the latest config value.

@shijie-oai shijie-oai marked this pull request as ready for review January 9, 2026 21:41
agent_status: Arc::clone(&agent_status),
state: Mutex::new(state),
features: config.features.clone(),
pending_mcp_server_refresh_config: Mutex::new(None),
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Track if there is any pending config, if there is refresh_mcp_servers_if_requested would use it to reinitialize the mcp server.

self.state.threads.read().await.keys().copied().collect()
}

pub async fn refresh_mcp_servers(&self, refresh_config: McpServerRefreshConfig) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Iterate through all active threads and submit Op::RefreshMcpServers with the latest config so that thread can pick up and reinit mcp connection managers on next turn.

Copy link
Contributor

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: cb1ee777bc

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

Comment on lines +2353 to +2355
let thread_manager = Arc::clone(&self.thread_manager);
tokio::spawn(async move {
thread_manager.refresh_mcp_servers(refresh_config).await;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Serialize refresh requests to avoid stale config

Because mcp_server_refresh spawns a detached task, two rapid mcpServer/refresh calls can run out of order. If the older refresh task is scheduled after the newer one, it will enqueue an Op::RefreshMcpServers with the stale config last, and the core handler overwrites pending_mcp_server_refresh_config on each thread with that older payload. In that scenario, threads reinitialize with the wrong MCP configuration until another refresh arrives. Consider awaiting thread_manager.refresh_mcp_servers or adding sequencing/version checks so newer config always wins.

Useful? React with 👍 / 👎.

}
};

let mcp_servers = match serde_json::to_value(&config.mcp_servers) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why serialize with serde_json::to_value? these types become Value which feels less useful/descriptive than the real types.

i think self.load_latest_config() will already throw an error if it fails to read the config on disk

return;
}
};
let store_mode = match serde_json::from_value::<OAuthCredentialsStoreMode>(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

- `model/list` — list available models (with reasoning effort options).
- `skills/list` — list skills for one or more `cwd` values (optional `forceReload`).
- `mcpServer/oauth/login` — start an OAuth login for a configured MCP server; returns an `authorization_url` and later emits `mcpServer/oauthLogin/completed` once the browser flow finishes.
- `mcpServer/refresh` — reinitialize MCP servers for loaded threads and refresh cached tool lists; returns `{}`.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's the use case? is it:

  • user updates mcp config in config.toml
  • we want to reload that config without restarting the server?

maybe worth expanding on when this should be used

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

watched your demo just now (lol), what do you think about calling this config/mcpServer/reload? kind of generalizes to config/<feature>/reload, or one day might be nice to have a whole config/reload

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ya I am a bit hesitant with calling it that because this method does not actually reload the config and tell active threads that there are new config and reload on the next turn.

#8957 (comment)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually will rename and make it clean in the readme and leave comments in code.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sounds good! yeah the fact that the actual reload is deferred feels like an implementation detail, but ultimately it's just reloading the config

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants