416
notify: tokio::sync::Notify,
417
}
418
419
+impl PromptCancellation {
420
+ /// Resolves once the turn has been cancelled. `notify_one` leaves a permit
421
+ /// behind when nobody is waiting, so a signal sent just before this is
422
+ /// polled is not lost.
423
+ async fn cancelled(&self) {
424
+ while !self.cancelled.load(Ordering::Acquire) {
425
+ self.notify.notified().await;
426
+ }
427
+ }
428
+}
429
+
430
enum DrainTurnError {
431
Backend(backend::BackendError),
432
Cancelled,
1035
}
1036
}
1037
1038
+/// One session's lock, held for a request on it.
1039
+///
1040
+/// Dropping it also drops the session's entry in `session_locks` when the
1041
+/// request leaves no such session behind, which is what `session/close` does
1042
+/// and what any request naming an id nobody opened does.
1043
+struct SessionGuard {
1044
+ guard: tokio::sync::OwnedMutexGuard<()>,
1045
+ agent: Arc<SiGitAgent>,
1046
+ key: String,
1047
+}
1048
+
1049
+impl Drop for SessionGuard {
1050
+ fn drop(&mut self) {
1051
+ if self.agent.sessions.lock().unwrap().contains_key(&self.key) {
1052
+ return;
1053
+ }
1054
+ let held = tokio::sync::OwnedMutexGuard::mutex(&self.guard);
1055
+ let mut locks = self.agent.session_locks.lock().unwrap();
1056
+ // Two handles mean the map's and this guard's. A request still queued
1057
+ // on the lock holds a third, and then the entry stays: taking it out
1058
+ // would give the next request for this id a fresh lock, and the two
1059
+ // would no longer wait for each other. The queued request drops the
1060
+ // entry when its own guard goes. `lock_session` clones under this
1061
+ // mutex, so the count cannot grow before the entry is removed.
1062
+ if locks
1063
+ .get(&self.key)
1064
+ .is_some_and(|lock| Arc::ptr_eq(lock, held) && Arc::strong_count(lock) == 2)
1065
+ {
1066
+ locks.remove(&self.key);
1067
+ }
1068
+ }
1069
+}
1070
+
1071
struct SiGitAgent {
1072
engine: Arc<ChatEngine>,
1073
/// The active inference backend. `LocalBackend` by default; swapped to an
1115
workspace_lock: Arc<tokio::sync::Mutex<()>>,
1116
/// One lock per session id, held for a whole request on that session, so
1117
/// two requests on the same thread keep the order they arrived in. Always
1074
- /// taken before `workspace_lock`, never while holding it.
1118
+ /// taken before `workspace_lock`, never while holding it. A closed
1119
+ /// session's entry is dropped by `SessionGuard`.
1120
session_locks: std::sync::Mutex<std::collections::HashMap<String, Arc<tokio::sync::Mutex<()>>>>,
1121
}
1122
2059
}
2060
}
2061
2017
- /// The lock that orders requests on one session (see `session_locks`).
2018
- fn session_lock(&self, session_id: &SessionId) -> Arc<tokio::sync::Mutex<()>> {
2019
- Arc::clone(
2062
+ /// Take the lock that orders requests on one session (see `session_locks`).
2063
+ async fn lock_session(self: &Arc<Self>, session_id: &SessionId) -> SessionGuard {
2064
+ let key = session_id.to_string();
2065
+ let lock = Arc::clone(
2066
self.session_locks
2067
.lock()
2068
.unwrap()
2023
- .entry(session_id.to_string())
2069
+ .entry(key.clone())
2070
.or_default(),
2025
- )
2071
+ );
2072
+ SessionGuard {
2073
+ guard: lock.lock_owned().await,
2074
+ agent: Arc::clone(self),
2075
+ key,
2076
+ }
2077
}
2078
2079
/// Keep the active session's parked metadata in step with a successful
2172
2173
/// Take `workspace_lock` back after a turn gave it up, and make the turn's
2174
/// session the installed one again if another was installed meanwhile.
2124
- async fn resume_workspace(&self, workspace: &mut WorkspaceHold, session_id: &SessionId) {
2175
+ ///
2176
+ /// A turn whose session cannot be reinstalled is cancelled: whatever it ran
2177
+ /// next would run in another session's directory and roots. The turn holds
2178
+ /// its session's lock, which `session/close` waits for, so nothing takes
2179
+ /// the session away today. The turn does not rely on that.
2180
+ async fn resume_workspace(
2181
+ &self,
2182
+ workspace: &mut WorkspaceHold,
2183
+ session_id: &SessionId,
2184
+ cancellation: &PromptCancellation,
2185
+ ) {
2186
if workspace.reacquire().await
2187
&& let Err(error) = self.activate_session(session_id).await
2188
{
2189
log::warn!(
2129
- "session({session_id}): could not reinstall mid-turn: {}",
2190
+ "session({session_id}): could not reinstall mid-turn, cancelling the turn: {}",
2191
error.message
2192
);
2193
+ cancellation.cancelled.store(true, Ordering::Release);
2194
+ cancellation.notify.notify_one();
2195
}
2196
}
2197
3032
&cancellation,
3033
)
3034
.await;
2972
- self.resume_workspace(&mut workspace, &session_id).await;
3035
+ self.resume_workspace(&mut workspace, &session_id, &cancellation)
3036
+ .await;
3037
let mut result = match outcome {
3038
Ok(result) => result,
3039
Err(DrainTurnError::Cancelled) => {
3222
if release_while_waiting {
3223
workspace.release();
3224
}
3161
- let verdict = self
3162
- .request_tool_permission(
3225
+ // A client that cancels the turn answers the
3226
+ // request itself. One that closes the session
3227
+ // need not, so the wait ends on either.
3228
+ let verdict = tokio::select! {
3229
+ verdict = self.request_tool_permission(
3230
cx,
3231
&session_id,
3232
&tc.id,
3233
&tc.name,
3234
&tc.arguments,
3168
- )
3235
+ ) => verdict,
3236
+ _ = cancellation.cancelled() => PermissionVerdict::TurnCancelled,
3237
+ };
3238
+ self.resume_workspace(&mut workspace, &session_id, &cancellation)
3239
.await;
3170
- self.resume_workspace(&mut workspace, &session_id).await;
3240
+ // An approval can cross a cancellation on the wire,
3241
+ // or arrive while this turn waited for the lock.
3242
+ let verdict = if cancellation.cancelled.load(Ordering::Acquire) {
3243
+ PermissionVerdict::TurnCancelled
3244
+ } else {
3245
+ verdict
3246
+ };
3247
match verdict {
3248
PermissionVerdict::Approved => {
3249
// The call the user just approved leaves
3384
&cancellation,
3385
)
3386
.await;
3311
- self.resume_workspace(&mut workspace, &session_id).await;
3387
+ self.resume_workspace(&mut workspace, &session_id, &cancellation)
3388
+ .await;
3389
result = match outcome {
3390
Ok(result) => result,
3391
Err(DrainTurnError::Cancelled) => {
5448
let state = Arc::clone(&state);
5449
let task_cx = cx.clone();
5450
cx.spawn(async move {
5374
- let session = state.session_lock(&req.session_id);
5375
- let _session = session.lock().await;
5451
+ let _session = state.lock_session(&req.session_id).await;
5452
let _workspace = state.workspace_lock.lock().await;
5453
handle_response(responder, state.handle_load_session(&task_cx, req).await)
5454
})
5463
let state = Arc::clone(&state);
5464
let task_cx = cx.clone();
5465
cx.spawn(async move {
5390
- let session = state.session_lock(&req.session_id);
5391
- let _session = session.lock().await;
5466
+ let _session = state.lock_session(&req.session_id).await;
5467
let _workspace = state.workspace_lock.lock().await;
5468
handle_response(responder, state.handle_resume_session(&task_cx, req).await)
5469
})
5480
state.begin_close(&req.session_id);
5481
let state = Arc::clone(&state);
5482
cx.spawn(async move {
5408
- let session = state.session_lock(&req.session_id);
5409
- let _session = session.lock().await;
5483
+ let _session = state.lock_session(&req.session_id).await;
5484
let _workspace = state.workspace_lock.lock().await;
5485
handle_response(responder, state.handle_close_session(req).await)
5486
})
5497
cx.spawn(async move {
5498
// The source's lock: a fork copies its conversation,
5499
// which has to be between turns.
5426
- let session = state.session_lock(&req.session_id);
5427
- let _session = session.lock().await;
5500
+ let _session = state.lock_session(&req.session_id).await;
5501
let _workspace = state.workspace_lock.lock().await;
5502
handle_response(responder, state.handle_fork_session(&task_cx, req).await)
5503
})
5526
let state = Arc::clone(&state);
5527
let task_cx = cx.clone();
5528
cx.spawn(async move {
5456
- let session = state.session_lock(&req.session_id);
5457
- let _session = session.lock().await;
5529
+ let _session = state.lock_session(&req.session_id).await;
5530
handle_response(responder, state.handle_prompt(&task_cx, req).await)
5531
})
5532
}
5542
let state = Arc::clone(&state);
5543
let task_cx = cx.clone();
5544
cx.spawn(async move {
5473
- let session = state.session_lock(&req.session_id);
5474
- let _session = session.lock().await;
5545
+ let _session = state.lock_session(&req.session_id).await;
5546
let _workspace = state.workspace_lock.lock().await;
5547
handle_response(
5548
responder,
5560
let state = Arc::clone(&state);
5561
let task_cx = cx.clone();
5562
cx.spawn(async move {
5492
- let session = state.session_lock(&req.session_id);
5493
- let _session = session.lock().await;
5563
+ let _session = state.lock_session(&req.session_id).await;
5564
let _workspace = state.workspace_lock.lock().await;
5565
handle_response(
5566
responder,
6460
std::fs::remove_dir_all(&temp).ok();
6461
}
6462
6463
+ #[tokio::test]
6464
+ #[allow(clippy::await_holding_lock)] // the lock guards process-global env for the whole test
6465
+ async fn a_session_lock_entry_goes_once_its_session_and_its_waiters_are_gone() {
6466
+ let _guard = crate::ENV_TEST_LOCK
6467
+ .lock()
6468
+ .unwrap_or_else(|poisoned| poisoned.into_inner());
6469
+ let agent = Arc::new(SiGitAgent::new(
6470
+ Arc::new(ChatEngine::new()),
6471
+ default_local_model_config(),
6472
+ Arc::new(AtomicBool::new(true)),
6473
+ Arc::new(AtomicBool::new(false)),
6474
+ Arc::new(std::sync::Mutex::new(None)),
6475
+ false,
6476
+ false,
6477
+ ));
6478
+ let held = |key: &str| agent.session_locks.lock().unwrap().contains_key(key);
6479
+ let thread = SessionId::new("lock-thread");
6480
+ agent.sessions.lock().unwrap().insert(
6481
+ "lock-thread".into(),
6482
+ agent.session_state(&std::env::temp_dir(), &[]),
6483
+ );
6484
+
6485
+ // An open session keeps its entry between requests.
6486
+ drop(agent.lock_session(&thread).await);
6487
+ assert!(held("lock-thread"));
6488
+
6489
+ // A request naming an id nobody opened leaves nothing behind.
6490
+ drop(agent.lock_session(&SessionId::new("lock-stranger")).await);
6491
+ assert!(!held("lock-stranger"));
6492
+
6493
+ // The session closes with another request queued behind the close.
6494
+ // That request still has to find the same lock, so the entry stays
6495
+ // until it has had its turn.
6496
+ let closing = agent.lock_session(&thread).await;
6497
+ let queued = agent.lock_session(&thread);
6498
+ tokio::pin!(queued);
6499
+ tokio::select! {
6500
+ biased;
6501
+ _ = &mut queued => panic!("the queued request ran beside the one holding the lock"),
6502
+ _ = std::future::ready(()) => {}
6503
+ }
6504
+ agent.sessions.lock().unwrap().remove("lock-thread");
6505
+ drop(closing);
6506
+ assert!(held("lock-thread"));
6507
+ drop(queued.await);
6508
+ assert!(!held("lock-thread"));
6509
+ }
6510
+
6511
+ #[tokio::test]
6512
+ #[allow(clippy::await_holding_lock)] // the lock guards process-global env for the whole test
6513
+ async fn a_turn_whose_session_cannot_be_reinstalled_is_cancelled() {
6514
+ let _guard = crate::ENV_TEST_LOCK
6515
+ .lock()
6516
+ .unwrap_or_else(|poisoned| poisoned.into_inner());
6517
+ let agent = SiGitAgent::new(
6518
+ Arc::new(ChatEngine::new()),
6519
+ default_local_model_config(),
6520
+ Arc::new(AtomicBool::new(true)),
6521
+ Arc::new(AtomicBool::new(false)),
6522
+ Arc::new(std::sync::Mutex::new(None)),
6523
+ false,
6524
+ false,
6525
+ );
6526
+ let gone = SessionId::new("resume-thread-gone");
6527
+ let cancellation = PromptCancellation::default();
6528
+ let mut workspace = WorkspaceHold::acquire(&agent.workspace_lock).await;
6529
+
6530
+ // The lock was never given up, so there is nothing to reinstall.
6531
+ agent
6532
+ .resume_workspace(&mut workspace, &gone, &cancellation)
6533
+ .await;
6534
+ assert!(!cancellation.cancelled.load(Ordering::Acquire));
6535
+
6536
+ workspace.release();
6537
+ agent
6538
+ .resume_workspace(&mut workspace, &gone, &cancellation)
6539
+ .await;
6540
+ assert!(cancellation.cancelled.load(Ordering::Acquire));
6541
+ // The signal is there for whatever waits on it next.
6542
+ cancellation.cancelled().await;
6543
+ assert!(agent.active_session.lock().unwrap().is_none());
6544
+ }
6545
+
6546
#[test]
6547
fn session_context_message_lists_every_root_of_a_multi_root_project() {
6548
let primary = PathBuf::from("/tmp/primary");