fix: session downgrade (#8196)

The problem is that the `tokio` task own an `Arc` reference of the
session and that this task only exit with the broadcast channel get
closed. But this never get closed if the session is not dropped. So it's
a snake biting his tail basically

The most notable result was that non of the `Drop` implementation were
triggered (temporary files, shell snapshots, session cleaning etc etc)
when closing the session (through a `/new` for example)

The fix is just to weaken the `Arc` and upgrade it on the fly
This commit is contained in:
jif-oai 2025-12-17 18:44:39 +00:00 committed by GitHub
parent 9f28c6251d
commit 167553f00d
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
2 changed files with 39 additions and 1 deletions

View file

@ -738,11 +738,14 @@ impl Session {
.services
.skills_manager
.subscribe_skills_update_notifications();
let sess = Arc::clone(&sess);
let sess = Arc::downgrade(&sess);
tokio::spawn(async move {
loop {
match rx.recv().await {
Ok(()) => {
let Some(sess) = sess.upgrade() else {
break;
};
let turn_context = sess.new_default_turn().await;
sess.send_event(turn_context.as_ref(), EventMsg::SkillsUpdateAvailable)
.await;

View file

@ -309,6 +309,41 @@ async fn shell_command_snapshot_still_intercepts_apply_patch() -> Result<()> {
Ok(())
}
#[cfg_attr(target_os = "windows", ignore)]
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
async fn shell_snapshot_deleted_after_shutdown_with_skills_enabled() -> Result<()> {
let builder = test_codex().with_config(|config| {
config.features.enable(Feature::ShellSnapshot);
config.features.enable(Feature::Skills);
});
let harness = TestCodexHarness::with_builder(builder).await?;
let home = harness.test().home.clone();
let codex_home = home.path().to_path_buf();
let codex = harness.test().codex.clone();
let mut entries = fs::read_dir(codex_home.join("shell_snapshots")).await?;
let snapshot_path = entries
.next_entry()
.await?
.map(|entry| entry.path())
.expect("shell snapshot created");
assert!(snapshot_path.exists());
codex.submit(Op::Shutdown {}).await?;
wait_for_event(&codex, |ev| matches!(ev, EventMsg::ShutdownComplete)).await;
drop(codex);
drop(harness);
assert_eq!(
snapshot_path.exists(),
false,
"snapshot should be removed after shutdown"
);
Ok(())
}
#[cfg_attr(not(target_os = "macos"), ignore)]
#[cfg_attr(
target_os = "macos",