fix: Features should be immutable over the lifetime of a session/thread (#7540)
I noticed that `features: Features` was defined on `struct SessionConfiguration`, which is commonly owned by `SessionState`, which is in turn owned by `Session`. Though I do not believe that `Features` should be allowed to be modified over the course of a session (if the feature state is not invariant, it makes it harder to reason about), which argues that it should live on `Session` rather than `SessionState` or `SessionConfiguration`. This PR moves `Features` to `Session` and updates all call sites. It appears the only place we were mutating `Features` was: - in tests - the sub-agent config for a review task:3ef76ff29d/codex-rs/core/src/tasks/review.rs (L86-L89)Note this change also means it is no longer an `async` call to check the state of a feature, eliminating the possibility of a [TOCTTOU](https://en.wikipedia.org/wiki/Time-of-check_to_time-of-use) error between checking the state of a feature and acting on it:3ef76ff29d/codex-rs/core/src/codex.rs (L1069-L1076)
This commit is contained in:
Michael Bolin
GitHub
parent
9a50a04400
commit
1cfc967eb8
3 changed files with 26 additions and 40 deletions
|
|
@ -12,6 +12,7 @@ use crate::compact::run_inline_auto_compact_task;
|
|||
use crate::compact::should_use_remote_compact_task;
|
||||
use crate::compact_remote::run_inline_remote_auto_compact_task;
|
||||
use crate::features::Feature;
|
||||
use crate::features::Features;
|
||||
use crate::function_tool::FunctionCallError;
|
||||
use crate::parse_command::parse_command;
|
||||
use crate::parse_turn_item;
|
||||
|
|
@ -190,7 +191,6 @@ impl Codex {
|
|||
sandbox_policy: config.sandbox_policy.clone(),
|
||||
cwd: config.cwd.clone(),
|
||||
original_config_do_not_use: Arc::clone(&config),
|
||||
features: config.features.clone(),
|
||||
exec_policy,
|
||||
session_source,
|
||||
};
|
||||
|
|
@ -264,6 +264,9 @@ pub(crate) struct Session {
|
|||
conversation_id: ConversationId,
|
||||
tx_event: Sender<Event>,
|
||||
state: Mutex<SessionState>,
|
||||
/// The set of enabled features should be invariant for the lifetime of the
|
||||
/// session.
|
||||
features: Features,
|
||||
pub(crate) active_turn: Mutex<Option<ActiveTurn>>,
|
||||
pub(crate) services: SessionServices,
|
||||
next_internal_sub_id: AtomicU64,
|
||||
|
|
@ -344,8 +347,6 @@ pub(crate) struct SessionConfiguration {
|
|||
/// operate deterministically.
|
||||
cwd: PathBuf,
|
||||
|
||||
/// Set of feature flags for this session
|
||||
features: Features,
|
||||
/// Execpolicy policy, applied only when enabled by feature flag.
|
||||
exec_policy: Arc<ExecPolicy>,
|
||||
|
||||
|
|
@ -401,6 +402,7 @@ impl Session {
|
|||
sub_id: String,
|
||||
) -> TurnContext {
|
||||
let config = session_configuration.original_config_do_not_use.clone();
|
||||
let features = &config.features;
|
||||
let model_family = find_family_for_model(&session_configuration.model)
|
||||
.unwrap_or_else(|| config.model_family.clone());
|
||||
let mut per_turn_config = (*config).clone();
|
||||
|
|
@ -408,6 +410,7 @@ impl Session {
|
|||
per_turn_config.model_family = model_family.clone();
|
||||
per_turn_config.model_reasoning_effort = session_configuration.model_reasoning_effort;
|
||||
per_turn_config.model_reasoning_summary = session_configuration.model_reasoning_summary;
|
||||
per_turn_config.features = features.clone();
|
||||
if let Some(model_info) = get_model_info(&model_family) {
|
||||
per_turn_config.model_context_window = Some(model_info.context_window);
|
||||
}
|
||||
|
|
@ -430,7 +433,7 @@ impl Session {
|
|||
|
||||
let tools_config = ToolsConfig::new(&ToolsConfigParams {
|
||||
model_family: &model_family,
|
||||
features: &config.features,
|
||||
features,
|
||||
});
|
||||
|
||||
TurnContext {
|
||||
|
|
@ -516,7 +519,7 @@ impl Session {
|
|||
|
||||
let mut post_session_configured_events = Vec::<Event>::new();
|
||||
|
||||
for (alias, feature) in session_configuration.features.legacy_feature_usages() {
|
||||
for (alias, feature) in config.features.legacy_feature_usages() {
|
||||
let canonical = feature.key();
|
||||
let summary = format!("`{alias}` is deprecated. Use `{canonical}` instead.");
|
||||
let details = if alias == canonical {
|
||||
|
|
@ -575,6 +578,7 @@ impl Session {
|
|||
conversation_id,
|
||||
tx_event: tx_event.clone(),
|
||||
state: Mutex::new(state),
|
||||
features: config.features.clone(),
|
||||
active_turn: Mutex::new(None),
|
||||
services,
|
||||
next_internal_sub_id: AtomicU64::new(0),
|
||||
|
|
@ -1046,7 +1050,7 @@ impl Session {
|
|||
}
|
||||
|
||||
pub(crate) async fn record_model_warning(&self, message: impl Into<String>, ctx: &TurnContext) {
|
||||
if !self.enabled(Feature::ModelWarnings).await {
|
||||
if !self.enabled(Feature::ModelWarnings) {
|
||||
return;
|
||||
}
|
||||
|
||||
|
|
@ -1075,13 +1079,8 @@ impl Session {
|
|||
self.persist_rollout_items(&rollout_items).await;
|
||||
}
|
||||
|
||||
pub async fn enabled(&self, feature: Feature) -> bool {
|
||||
self.state
|
||||
.lock()
|
||||
.await
|
||||
.session_configuration
|
||||
.features
|
||||
.enabled(feature)
|
||||
pub fn enabled(&self, feature: Feature) -> bool {
|
||||
self.features.enabled(feature)
|
||||
}
|
||||
|
||||
async fn send_raw_response_items(&self, turn_context: &TurnContext, items: &[ResponseItem]) {
|
||||
|
|
@ -1264,7 +1263,7 @@ impl Session {
|
|||
turn_context: Arc<TurnContext>,
|
||||
cancellation_token: CancellationToken,
|
||||
) {
|
||||
if !self.enabled(Feature::GhostCommit).await {
|
||||
if !self.enabled(Feature::GhostCommit) {
|
||||
return;
|
||||
}
|
||||
let token = match turn_context.tool_call_gate.subscribe().await {
|
||||
|
|
@ -1852,7 +1851,7 @@ async fn spawn_review_thread(
|
|||
let review_model_family = find_family_for_model(&model)
|
||||
.unwrap_or_else(|| parent_turn_context.client.get_model_family());
|
||||
// For reviews, disable web_search and view_image regardless of global settings.
|
||||
let mut review_features = config.features.clone();
|
||||
let mut review_features = sess.features.clone();
|
||||
review_features
|
||||
.disable(crate::features::Feature::WebSearchRequest)
|
||||
.disable(crate::features::Feature::ViewImageTool);
|
||||
|
|
@ -1873,6 +1872,7 @@ async fn spawn_review_thread(
|
|||
per_turn_config.model_family = model_family.clone();
|
||||
per_turn_config.model_reasoning_effort = Some(ReasoningEffortConfig::Low);
|
||||
per_turn_config.model_reasoning_summary = ReasoningSummaryConfig::Detailed;
|
||||
per_turn_config.features = review_features.clone();
|
||||
if let Some(model_info) = get_model_info(&model_family) {
|
||||
per_turn_config.model_context_window = Some(model_info.context_window);
|
||||
}
|
||||
|
|
@ -2020,7 +2020,7 @@ pub(crate) async fn run_task(
|
|||
|
||||
// as long as compaction works well in getting us way below the token limit, we shouldn't worry about being in an infinite loop.
|
||||
if token_limit_reached {
|
||||
if should_use_remote_compact_task(&sess).await {
|
||||
if should_use_remote_compact_task(&sess) {
|
||||
run_inline_remote_auto_compact_task(sess.clone(), turn_context.clone())
|
||||
.await;
|
||||
} else {
|
||||
|
|
@ -2103,14 +2103,7 @@ async fn run_turn(
|
|||
.supports_parallel_tool_calls;
|
||||
|
||||
// TODO(jif) revert once testing phase is done.
|
||||
let parallel_tool_calls = model_supports_parallel
|
||||
&& sess
|
||||
.state
|
||||
.lock()
|
||||
.await
|
||||
.session_configuration
|
||||
.features
|
||||
.enabled(Feature::ParallelToolCalls);
|
||||
let parallel_tool_calls = model_supports_parallel && sess.enabled(Feature::ParallelToolCalls);
|
||||
let mut base_instructions = turn_context.base_instructions.clone();
|
||||
if parallel_tool_calls {
|
||||
static INSTRUCTIONS: &str = include_str!("../templates/parallel/instructions.md");
|
||||
|
|
@ -2486,7 +2479,6 @@ pub(super) fn get_last_assistant_message_from_turn(responses: &[ResponseItem]) -
|
|||
})
|
||||
}
|
||||
|
||||
use crate::features::Features;
|
||||
#[cfg(test)]
|
||||
pub(crate) use tests::make_session_and_context;
|
||||
|
||||
|
|
@ -2599,7 +2591,6 @@ mod tests {
|
|||
sandbox_policy: config.sandbox_policy.clone(),
|
||||
cwd: config.cwd.clone(),
|
||||
original_config_do_not_use: Arc::clone(&config),
|
||||
features: Features::default(),
|
||||
exec_policy: Arc::new(ExecPolicy::empty()),
|
||||
session_source: SessionSource::Exec,
|
||||
};
|
||||
|
|
@ -2798,7 +2789,6 @@ mod tests {
|
|||
sandbox_policy: config.sandbox_policy.clone(),
|
||||
cwd: config.cwd.clone(),
|
||||
original_config_do_not_use: Arc::clone(&config),
|
||||
features: Features::default(),
|
||||
exec_policy: Arc::new(ExecPolicy::empty()),
|
||||
session_source: SessionSource::Exec,
|
||||
};
|
||||
|
|
@ -2831,6 +2821,7 @@ mod tests {
|
|||
conversation_id,
|
||||
tx_event,
|
||||
state: Mutex::new(state),
|
||||
features: config.features.clone(),
|
||||
active_turn: Mutex::new(None),
|
||||
services,
|
||||
next_internal_sub_id: AtomicU64::new(0),
|
||||
|
|
@ -2876,7 +2867,6 @@ mod tests {
|
|||
sandbox_policy: config.sandbox_policy.clone(),
|
||||
cwd: config.cwd.clone(),
|
||||
original_config_do_not_use: Arc::clone(&config),
|
||||
features: Features::default(),
|
||||
exec_policy: Arc::new(ExecPolicy::empty()),
|
||||
session_source: SessionSource::Exec,
|
||||
};
|
||||
|
|
@ -2909,6 +2899,7 @@ mod tests {
|
|||
conversation_id,
|
||||
tx_event,
|
||||
state: Mutex::new(state),
|
||||
features: config.features.clone(),
|
||||
active_turn: Mutex::new(None),
|
||||
services,
|
||||
next_internal_sub_id: AtomicU64::new(0),
|
||||
|
|
@ -2919,15 +2910,10 @@ mod tests {
|
|||
|
||||
#[tokio::test]
|
||||
async fn record_model_warning_appends_user_message() {
|
||||
let (session, turn_context) = make_session_and_context();
|
||||
|
||||
session
|
||||
.state
|
||||
.lock()
|
||||
.await
|
||||
.session_configuration
|
||||
.features
|
||||
.enable(Feature::ModelWarnings);
|
||||
let (mut session, turn_context) = make_session_and_context();
|
||||
let mut features = Features::with_defaults();
|
||||
features.enable(Feature::ModelWarnings);
|
||||
session.features = features;
|
||||
|
||||
session
|
||||
.record_model_warning("too many unified exec sessions", &turn_context)
|
||||
|
|
|
|||
|
|
@ -32,13 +32,13 @@ pub const SUMMARIZATION_PROMPT: &str = include_str!("../templates/compact/prompt
|
|||
pub const SUMMARY_PREFIX: &str = include_str!("../templates/compact/summary_prefix.md");
|
||||
const COMPACT_USER_MESSAGE_MAX_TOKENS: usize = 20_000;
|
||||
|
||||
pub(crate) async fn should_use_remote_compact_task(session: &Session) -> bool {
|
||||
pub(crate) fn should_use_remote_compact_task(session: &Session) -> bool {
|
||||
session
|
||||
.services
|
||||
.auth_manager
|
||||
.auth()
|
||||
.is_some_and(|auth| auth.mode == AuthMode::ChatGPT)
|
||||
&& session.enabled(Feature::RemoteCompaction).await
|
||||
&& session.enabled(Feature::RemoteCompaction)
|
||||
}
|
||||
|
||||
pub(crate) async fn run_inline_auto_compact_task(
|
||||
|
|
|
|||
|
|
@ -25,7 +25,7 @@ impl SessionTask for CompactTask {
|
|||
_cancellation_token: CancellationToken,
|
||||
) -> Option<String> {
|
||||
let session = session.clone_session();
|
||||
if crate::compact::should_use_remote_compact_task(&session).await {
|
||||
if crate::compact::should_use_remote_compact_task(&session) {
|
||||
crate::compact_remote::run_remote_compact_task(session, ctx).await
|
||||
} else {
|
||||
crate::compact::run_compact_task(session, ctx, input).await
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue