Skip to content

Commit eeaedd9

Browse files
committed
fix(acp): poison interrupted permission writes
Signed-off-by: KC <79471844+wolfyy970@users.noreply.github.com>
1 parent a83200b commit eeaedd9

1 file changed

Lines changed: 160 additions & 1 deletion

File tree

crates/buzz-acp/src/acp.rs

Lines changed: 160 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -202,6 +202,19 @@ struct PermissionEntry {
202202
deadline: tokio::time::Instant,
203203
}
204204

205+
/// Permission response whose bytes may be partially written to the agent.
206+
///
207+
/// This marker is stored on [`AcpClient`] before the first write await and is
208+
/// cleared only after that write completes. If the enclosing prompt future is
209+
/// cancelled, the marker survives the dropped future so cleanup can retire the
210+
/// UI request and replace the process without writing a second response.
211+
#[derive(Debug, Clone)]
212+
struct PermissionWriteInProgress {
213+
request_id: serde_json::Value,
214+
pending_entry_key: Option<String>,
215+
nonce: String,
216+
}
217+
205218
/// ACP client that owns an agent subprocess and communicates over its stdio.
206219
///
207220
/// One `AcpClient` per agent process. Multiple sessions can be created on the
@@ -245,6 +258,11 @@ pub struct AcpClient {
245258
/// When `true` the process MUST NOT be returned to the pool — it must be
246259
/// respawned. The cancel path surfaces this via `PermissionPoisoned`.
247260
permission_poisoned: bool,
261+
/// Cancellation-persistent marker for every permission response write.
262+
///
263+
/// Unlike a stack guard, this remains set when `select!` drops the prompt
264+
/// future in the middle of an awaited stdin write.
265+
permission_write_in_progress: Option<PermissionWriteInProgress>,
248266
/// Resolved permission configuration. Determines how `handle_permission_request`
249267
/// answers ACP `session/request_permission` frames.
250268
permission_config: ResolvedPermissionConfig,
@@ -650,6 +668,7 @@ impl AcpClient {
650668
permission_responded: false,
651669
pending_permissions: std::collections::HashMap::new(),
652670
permission_poisoned: false,
671+
permission_write_in_progress: None,
653672
permission_config: ResolvedPermissionConfig {
654673
policy: crate::config::PermissionPolicy::Reject,
655674
effective_mode: PermissionMode::DontAsk,
@@ -1188,6 +1207,34 @@ impl AcpClient {
11881207
AcpError::Protocol("cancel_with_cleanup called with no in-flight prompt".into())
11891208
})?;
11901209

1210+
// The prompt future may have been dropped while a permission response
1211+
// was partway through its three awaited stdin writes. The adapter may
1212+
// have received any prefix of that response, so writing a cancellation
1213+
// response would violate JSON-RPC's one-response rule. Retire the UI
1214+
// request, poison the process, and let the pool replace it.
1215+
if let Some(write) = self.permission_write_in_progress.take() {
1216+
if let Some(entry_key) = write.pending_entry_key.as_ref() {
1217+
self.pending_permissions.remove(entry_key);
1218+
}
1219+
self.pending_permission_id = None;
1220+
self.permission_responded = false;
1221+
self.permission_poisoned = true;
1222+
self.observe_authorized(
1223+
"permission_terminal",
1224+
AuthorizationEnvelope {
1225+
request_nonce: write.nonce,
1226+
actionable: false,
1227+
reason: Some("uncertain".to_string()),
1228+
},
1229+
serde_json::json!({ "id": write.request_id }),
1230+
);
1231+
tracing::error!(
1232+
target: "acp::cancel",
1233+
"cancel interrupted a permission response write — poisoning process"
1234+
);
1235+
return Err(AcpError::PermissionPoisoned);
1236+
}
1237+
11911238
// Check for poisoning first: if a permission write is in progress we
11921239
// must not send any more bytes to this process — return the dedicated
11931240
// error so `classify_control_cancel_failure` triggers respawn.
@@ -1377,6 +1424,21 @@ impl AcpClient {
13771424
) -> bool {
13781425
let (id_str, id_val) = entry;
13791426
let (nonce, reason, response) = outcome;
1427+
let Some(pending_entry) = self.pending_permissions.get_mut(id_str) else {
1428+
tracing::error!(
1429+
target: "acp::permission",
1430+
"permission id={id_val} disappeared before response write — poisoning process"
1431+
);
1432+
self.permission_poisoned = true;
1433+
return false;
1434+
};
1435+
pending_entry.state = PermissionEntryState::Writing;
1436+
self.permission_write_in_progress = Some(PermissionWriteInProgress {
1437+
request_id: id_val.clone(),
1438+
pending_entry_key: Some(id_str.to_string()),
1439+
nonce: nonce.to_string(),
1440+
});
1441+
13801442
// Write the response. Use a bounded timeout when one is provided.
13811443
let write_result = if let Some(deadline) = write_deadline {
13821444
tokio::time::timeout_at(deadline, self.write_ndjson_no_observe(&response))
@@ -1387,6 +1449,7 @@ impl AcpClient {
13871449
} else {
13881450
self.write_ndjson_no_observe(&response).await
13891451
};
1452+
self.permission_write_in_progress = None;
13901453

13911454
match write_result {
13921455
Ok(()) => {
@@ -1464,7 +1527,15 @@ impl AcpClient {
14641527
reason: &str,
14651528
response: serde_json::Value,
14661529
) -> Result<(), AcpError> {
1467-
match self.write_ndjson_no_observe(&response).await {
1530+
self.permission_write_in_progress = Some(PermissionWriteInProgress {
1531+
request_id: id_val.clone(),
1532+
pending_entry_key: None,
1533+
nonce: nonce.to_string(),
1534+
});
1535+
let write_result = self.write_ndjson_no_observe(&response).await;
1536+
self.permission_write_in_progress = None;
1537+
1538+
match write_result {
14681539
Ok(()) => {
14691540
self.observe_authorized(
14701541
"acp_write",
@@ -7375,6 +7446,94 @@ mod tests {
73757446
});
73767447
}
73777448

7449+
#[tokio::test]
7450+
async fn cancel_mid_permission_write_sends_no_second_response_and_poisons_process() {
7451+
// A child that keeps stdin open without reading eventually backpressures
7452+
// the real OS pipe. Dropping the permission future at that point mirrors
7453+
// `run_prompt_task` selecting a control signal over the prompt future.
7454+
let mut client = spawn_script("sleep 30").await;
7455+
let observer = crate::observer::ObserverHandle::in_process();
7456+
client.set_observer(Some(observer.clone()), 0);
7457+
let attempt_counter = std::sync::Arc::new(std::sync::atomic::AtomicUsize::new(0));
7458+
client.set_write_attempt_count(attempt_counter.clone());
7459+
7460+
let request_id = serde_json::json!(77);
7461+
let nonce = "cancel-mid-write";
7462+
client.pending_permission_id = Some(request_id.clone());
7463+
client.permission_responded = false;
7464+
client.last_prompt_id = Some(999);
7465+
7466+
// Larger than an OS pipe, so the single production write blocks after
7467+
// some bytes have been accepted by the child stdin.
7468+
let response = serde_json::json!({
7469+
"jsonrpc": "2.0",
7470+
"id": request_id.clone(),
7471+
"result": {
7472+
"outcome": {
7473+
"outcome": "selected",
7474+
"optionId": "x".repeat(2 * 1024 * 1024),
7475+
}
7476+
}
7477+
});
7478+
7479+
tokio::select! {
7480+
biased;
7481+
result = client.finish_permission_sync(
7482+
&request_id,
7483+
nonce,
7484+
"allowed",
7485+
response,
7486+
) => panic!("backpressured permission write completed unexpectedly: {result:?}"),
7487+
() = async {
7488+
while attempt_counter.load(std::sync::atomic::Ordering::Relaxed) == 0 {
7489+
tokio::task::yield_now().await;
7490+
}
7491+
} => {}
7492+
}
7493+
7494+
assert!(
7495+
client.permission_write_in_progress.is_some(),
7496+
"dropping the write future must leave a cancellation-persistent marker"
7497+
);
7498+
7499+
let error = client
7500+
.cancel_with_cleanup_grace("sess-mid-write", std::time::Duration::from_millis(200))
7501+
.await
7502+
.expect_err("uncertain permission write must force process replacement");
7503+
assert!(matches!(error, AcpError::PermissionPoisoned));
7504+
assert!(client.permission_poisoned);
7505+
assert_eq!(
7506+
attempt_counter.load(std::sync::atomic::Ordering::Relaxed),
7507+
1,
7508+
"cancel must not attempt a second JSON-RPC response"
7509+
);
7510+
assert!(client.pending_permission_id.is_none());
7511+
7512+
let events = observer.snapshot();
7513+
assert_eq!(
7514+
events
7515+
.iter()
7516+
.filter(|event| {
7517+
event.kind == "permission_terminal"
7518+
&& event
7519+
.authorization
7520+
.as_ref()
7521+
.is_some_and(|auth| auth.reason.as_deref() == Some("uncertain"))
7522+
})
7523+
.count(),
7524+
1,
7525+
"Desktop must receive one uncertain terminal for the interrupted request"
7526+
);
7527+
assert_eq!(
7528+
events
7529+
.iter()
7530+
.filter(|event| event.kind == "acp_write" && event.authorization.is_some())
7531+
.count(),
7532+
0,
7533+
"an interrupted response must never be reported as delivered"
7534+
);
7535+
}
7536+
73787537
#[test]
73797538
fn poisoned_process_surfaces_immediately_on_next_cancel() {
73807539
// Once poisoned, every subsequent cancel must immediately return PermissionPoisoned

0 commit comments

Comments
 (0)