fix(acp): propagate agent cleanup errors on session delete

[Abhijay007]

Summary

This PR stops swallowing errors when tearing down in-memory agents during ACP session delete, archive, and close. It adds remove_session_if_loaded so cleanup is skipped when no agent is loaded and failures are propagated when one is, following up on the session lifecycle work in #7857.

Testing

mannual

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d4c249cabb

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[DOsinga]

Nice fix — propagating these cleanup errors (and treating "not loaded" as a non-error) is the right call, and doing the check-and-pop atomically under the write lock is clean.

Two things before I approve:

1. Drop the bool. Every call site discards the return value — no caller branches on whether an agent was actually loaded. Make it Result<()> with the not-loaded case being Ok(()). That's all any caller actually needs.

2. Trim the comments/test that only describe the discarded bool. The third sentence of the doc comment ("Concurrent callers... only the first removal returns Ok(true); the rest get Ok(false)") is describing an internal detail that no caller observes, and it goes away with the bool. Similarly test_concurrent_remove_session_if_loaded only asserts that internal atomicity via the bool — since no caller observes it, it's testing the implementation rather than a contract. I'd drop that test and keep test_remove_session_if_loaded, which does verify the observable behavior (not-loaded is Ok, not Err).

With those, happy to approve.

[Abhijay007]

Nice fix — propagating these cleanup errors (and treating "not loaded" as a non-error) is the right call, and doing the check-and-pop atomically under the write lock is clean.

Two things before I approve:

1. Drop the bool. Every call site discards the return value — no caller branches on whether an agent was actually loaded. Make it Result<()> with the not-loaded case being Ok(()). That's all any caller actually needs.
2. Trim the comments/test that only describe the discarded bool. The third sentence of the doc comment ("Concurrent callers... only the first removal returns Ok(true); the rest get Ok(false)") is describing an internal detail that no caller observes, and it goes away with the bool. Similarly test_concurrent_remove_session_if_loaded only asserts that internal atomicity via the bool — since no caller observes it, it's testing the implementation rather than a contract. I'd drop that test and keep test_remove_session_if_loaded, which does verify the observable behavior (not-loaded is Ok, not Err).

With those, happy to approve.

Hi @DOsinga thanks for the review, updated it

[michaelneale]

thanks - I can approve it as addressed