From 9d53dd91b5b5411dbfa8366fe7fd352ab3b1ddec Mon Sep 17 00:00:00 2001 From: Bryan Ashley Date: Thu, 1 Oct 2026 20:10:28 +0000 Subject: [PATCH] Prevent shared instruction providers from delegating to themselves (#50093) ## Why Reloading an unloaded root from a surviving child can supply the shared instruction wrapper already registered for that root. Replacing its underlying provider with that same wrapper makes instruction loading recurse into itself. ## What changed Reuse the existing wrapper without replacing its provider when both are the same `Arc`. Continue to accept distinct replacement providers. ## Testing Extend regression coverage to reload the root from a surviving child and verify that shared instructions still update. Check wrapper reuse, replacement with a distinct shared provider, and freezing the shared snapshot when switching to a provider that does not share with subagents. GitOrigin-RevId: 7b6b2f9031f206e203f6fa14e1478a36b305ec39 --- codex-rs/core/src/agent/control_tests.rs | 72 +++++++++++++++---- .../src/thread_manager/shared_instructions.rs | 10 ++- 2 files changed, 68 insertions(+), 14 deletions(-) diff --git a/codex-rs/core/src/agent/control_tests.rs b/codex-rs/core/src/agent/control_tests.rs index de1291c91e..2e2735ba21 100644 --- a/codex-rs/core/src/agent/control_tests.rs +++ b/codex-rs/core/src/agent/control_tests.rs @@ -1452,10 +1452,19 @@ async fn cold_resume_with_thread_instructions_preserves_lazy_v2_child_inheritanc ); } -#[test_case::test_case(false; "snapshot_is_not_shared")] -#[test_case::test_case(true; "shared_provider_survives")] +enum InstructionsReloadTarget { + Sibling, + Root, +} + +#[test_case::test_case(false, InstructionsReloadTarget::Sibling; "snapshot_is_not_shared")] +#[test_case::test_case(true, InstructionsReloadTarget::Sibling; "shared_provider_survives")] +#[test_case::test_case(true, InstructionsReloadTarget::Root; "root_reuses_surviving_shared_provider")] #[tokio::test] -async fn v2_sibling_reload_preserves_shared_instructions_after_root_unloads(shared: bool) { +async fn v2_reload_preserves_shared_instructions_after_root_unloads( + shared: bool, + reload_target: InstructionsReloadTarget, +) { let (home, mut config) = test_config().await; config .features @@ -1494,10 +1503,14 @@ async fn v2_sibling_reload_preserves_shared_instructions_after_root_unloads(shar spawn_v2_reload_test_child(control, harness.config.clone(), &root.thread, "sender") .await .thread_id; - let target_id = - spawn_v2_reload_test_child(control, harness.config.clone(), &root.thread, "target") - .await - .thread_id; + let target_id = match reload_target { + InstructionsReloadTarget::Root => root.thread_id, + InstructionsReloadTarget::Sibling => { + spawn_v2_reload_test_child(control, harness.config.clone(), &root.thread, "target") + .await + .thread_id + } + }; let sender = harness .manager .get_thread(sender_id) @@ -1510,9 +1523,15 @@ async fn v2_sibling_reload_preserves_shared_instructions_after_root_unloads(shar .expect("loaded target"); persist_thread_for_tree_resume(&root.thread, "root persisted").await; persist_thread_for_tree_resume(&sender, "sender persisted").await; - persist_thread_for_tree_resume(&target, "target persisted").await; - target.shutdown_and_wait().await.expect("shut down target"); - assert!(harness.manager.remove_thread(&target_id).await.is_some()); + if matches!(reload_target, InstructionsReloadTarget::Sibling) { + persist_thread_for_tree_resume(&target, "target persisted").await; + target.shutdown_and_wait().await.expect("shut down target"); + assert!(harness.manager.remove_thread(&target_id).await.is_some()); + } + root.thread + .shutdown_and_wait() + .await + .expect("shut down root"); assert!( harness .manager @@ -1537,7 +1556,7 @@ async fn v2_sibling_reload_preserves_shared_instructions_after_root_unloads(shar resume_config: crate::agent::child_config::build_agent_resume_config(&sender_turn) .expect("capture resume config"), input: crate::AgentInput::Message { - message: AgentMessage::Plaintext("wake the sibling".to_string()), + message: AgentMessage::Plaintext("wake the unloaded target".to_string()), mode: MessageDeliveryMode::QueueOnly, }, start_options: TurnStartOptions { @@ -1548,7 +1567,7 @@ async fn v2_sibling_reload_preserves_shared_instructions_after_root_unloads(shar }, }) .await - .expect("reload target from its sibling"); + .expect("reload target from a surviving child"); let resumed = harness .manager .get_thread(target_id) @@ -1612,6 +1631,34 @@ async fn resumed_root_reuses_or_freezes_surviving_shared_instructions() { .root_thread_instructions_provider(id, /*provider*/ None) .expect("reuse the live tree's provider"); assert!(Arc::ptr_eq(&shared, &reused)); + assert_eq!(reused.load_thread_instructions().await, snapshot); + + let reused = harness + .manager + .agent_control() + .runtime + .root_thread_instructions_provider(id, Some(reused)) + .expect("reuse the shared wrapper supplied during root reload"); + assert!(Arc::ptr_eq(&shared, &reused)); + *original.text.write().expect("update shared provider") = "updated shared instructions"; + assert_eq!( + reused.load_thread_instructions().await, + original.load_thread_instructions().await + ); + + let replacement = Arc::new(TestThreadInstructionsProvider { + text: "replacement shared instructions".into(), + shared: true, + }); + let replaced = harness + .manager + .agent_control() + .runtime + .root_thread_instructions_provider(id, Some(replacement.clone())) + .expect("replace the underlying provider"); + assert!(Arc::ptr_eq(&shared, &replaced)); + let snapshot = replacement.load_thread_instructions().await; + assert_eq!(reused.load_thread_instructions().await, snapshot); let private = Arc::new(TestThreadInstructionsProvider { text: "private replacement".into(), @@ -1624,6 +1671,7 @@ async fn resumed_root_reuses_or_freezes_surviving_shared_instructions() { .root_thread_instructions_provider(id, Some(private.clone())) .expect("private root provider"); *original.text.write().expect("update old provider") = "stale shared update"; + *replacement.text.write().expect("update replaced provider") = "another stale shared update"; assert_eq!(shared.load_thread_instructions().await, snapshot); assert_eq!( root_only.load_thread_instructions().await, diff --git a/codex-rs/core/src/thread_manager/shared_instructions.rs b/codex-rs/core/src/thread_manager/shared_instructions.rs index 4ec4549359..066dbf03c6 100644 --- a/codex-rs/core/src/thread_manager/shared_instructions.rs +++ b/codex-rs/core/src/thread_manager/shared_instructions.rs @@ -1,4 +1,5 @@ //! Keeps one replaceable instruction provider per root while its agent tree is alive. +//! Reusing the shared wrapper must never make it delegate to itself. use codex_extension_api::Instructions; use codex_extension_api::LoadInstructionsFuture; @@ -38,10 +39,15 @@ impl SharedThreadInstructionsProviders { } match (existing, provider) { (Some(existing), provider) => { - if provider.is_some() { + // An unloaded root can inherit this wrapper from its surviving children. + let shared: Arc = existing.clone(); + if provider + .as_ref() + .is_some_and(|provider| !Arc::ptr_eq(provider, &shared)) + { existing.replace(provider); } - Some(existing) + Some(shared) } (None, Some(provider)) => { let shared = Arc::new(SharedThreadInstructionsProvider(Mutex::new(