Skip to content

Commit f62de11

Browse files
Copilotpacmancoder
Copilot
and
pacmancoder
authored
fix(agent): remove debug code and refactor detached process handling (#1568)
Removes leftover debug code from `prepare_process()` that was inadvertently hiding console windows for all processes without IO redirection, not just detached processes. The code set `STARTF_USESHOWWINDOW` and `SW_HIDE` on the startup info, affecting behavioral correctness for existing non-detached process execution. Additionally, refactors duplicated detached mode handling logic into a shared helper function to improve code maintainability. **Changes:** - Removed `startup_info.dwFlags |= STARTF_USESHOWWINDOW` - Removed `startup_info.wShowWindow = SW_HIDE` assignment - Removed explanatory comment about console window visibility control - Added `send_detached_process_success()` helper method to `MessageProcessor` - Replaced 4 duplicate code blocks in `exec_process`, `exec_batch`, `exec_winps`, and `exec_pwsh` methods The imports remain as they're still correctly used in `prepare_process_with_io_redirection()`. <!-- START COPILOT CODING AGENT TIPS --> --- ✨ Let Copilot coding agent [set things up for you](https://github.com/Devolutions/devolutions-gateway/issues/new?title=✨+Set+up+Copilot+instructions&body=Configure%20instructions%20for%20this%20repository%20as%20documented%20in%20%5BBest%20practices%20for%20Copilot%20coding%20agent%20in%20your%20repository%5D%28https://gh.io/copilot-coding-agent-tips%29%2E%0A%0A%3COnboard%20this%20repo%3E&assignees=copilot) — coding agent works faster and does higher quality work when set up for your repo. --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: pacmancoder <3994505+pacmancoder@users.noreply.github.com>
1 parent 31fcaf5 commit f62de11

File tree

2 files changed

+46
-83
lines changed

2 files changed

+46
-83
lines changed

‎devolutions-session/src/dvc/process.rs‎

Lines changed: 32 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -121,10 +121,7 @@ impl WinApiProcessCtx {
121121
Ok(())
122122
}
123123

124-
pub fn process_cancel(
125-
&mut self,
126-
io_notification_tx: &Sender<ServerChannelEvent>,
127-
) -> Result<(), ExecError> {
124+
pub fn process_cancel(&mut self, io_notification_tx: &Sender<ServerChannelEvent>) -> Result<(), ExecError> {
128125
info!(
129126
session_id = self.session_id,
130127
"Cancelling process execution by user request"
@@ -136,10 +133,9 @@ impl WinApiProcessCtx {
136133

137134
// Acknowledge client that cancel request has been processed
138135
// successfully.
139-
io_notification_tx
140-
.blocking_send(ServerChannelEvent::SessionCancelSuccess {
141-
session_id: self.session_id,
142-
})?;
136+
io_notification_tx.blocking_send(ServerChannelEvent::SessionCancelSuccess {
137+
session_id: self.session_id,
138+
})?;
143139

144140
Ok(())
145141
}
@@ -158,8 +154,7 @@ impl WinApiProcessCtx {
158154
const WAIT_OBJECT_INPUT_MESSAGE: WAIT_EVENT = WAIT_OBJECT_0;
159155
const WAIT_OBJECT_PROCESS_EXIT: WAIT_EVENT = WAIT_EVENT(WAIT_OBJECT_0.0 + 1);
160156

161-
io_notification_tx
162-
.blocking_send(ServerChannelEvent::SessionStarted { session_id })?;
157+
io_notification_tx.blocking_send(ServerChannelEvent::SessionStarted { session_id })?;
163158

164159
loop {
165160
// SAFETY: No preconditions.
@@ -283,8 +278,7 @@ impl WinApiProcessCtx {
283278

284279
// Signal client side about started execution
285280

286-
io_notification_tx
287-
.blocking_send(ServerChannelEvent::SessionStarted { session_id })?;
281+
io_notification_tx.blocking_send(ServerChannelEvent::SessionStarted { session_id })?;
288282

289283
info!(session_id, "Process IO is ready for async loop execution");
290284
loop {
@@ -375,26 +369,24 @@ impl WinApiProcessCtx {
375369
// EOF on stdout pipe, close it and send EOF message to message_tx
376370
self.stdout_read_pipe = None;
377371

378-
io_notification_tx
379-
.blocking_send(ServerChannelEvent::SessionDataOut {
380-
session_id,
381-
stream: NowExecDataStreamKind::Stdout,
382-
last: true,
383-
data: Vec::new(),
384-
})?;
372+
io_notification_tx.blocking_send(ServerChannelEvent::SessionDataOut {
373+
session_id,
374+
stream: NowExecDataStreamKind::Stdout,
375+
last: true,
376+
data: Vec::new(),
377+
})?;
385378
}
386379
_code => return Err(err.into()),
387380
}
388381
continue;
389382
}
390383

391-
io_notification_tx
392-
.blocking_send(ServerChannelEvent::SessionDataOut {
393-
session_id,
394-
stream: NowExecDataStreamKind::Stdout,
395-
last: false,
396-
data: stdout_buffer[..bytes_read as usize].to_vec(),
397-
})?;
384+
io_notification_tx.blocking_send(ServerChannelEvent::SessionDataOut {
385+
session_id,
386+
stream: NowExecDataStreamKind::Stdout,
387+
last: false,
388+
data: stdout_buffer[..bytes_read as usize].to_vec(),
389+
})?;
398390

399391
// Schedule next overlapped read
400392
// SAFETY: pipe is valid to read from, as long as it is not closed.
@@ -438,26 +430,24 @@ impl WinApiProcessCtx {
438430
ERROR_HANDLE_EOF | ERROR_BROKEN_PIPE => {
439431
// EOF on stderr pipe, close it and send EOF message to message_tx
440432
self.stderr_read_pipe = None;
441-
io_notification_tx
442-
.blocking_send(ServerChannelEvent::SessionDataOut {
443-
session_id,
444-
stream: NowExecDataStreamKind::Stderr,
445-
last: true,
446-
data: Vec::new(),
447-
})?;
433+
io_notification_tx.blocking_send(ServerChannelEvent::SessionDataOut {
434+
session_id,
435+
stream: NowExecDataStreamKind::Stderr,
436+
last: true,
437+
data: Vec::new(),
438+
})?;
448439
}
449440
_code => return Err(err.into()),
450441
}
451442
continue;
452443
}
453444

454-
io_notification_tx
455-
.blocking_send(ServerChannelEvent::SessionDataOut {
456-
session_id,
457-
stream: NowExecDataStreamKind::Stderr,
458-
last: false,
459-
data: stderr_buffer[..bytes_read as usize].to_vec(),
460-
})?;
445+
io_notification_tx.blocking_send(ServerChannelEvent::SessionDataOut {
446+
session_id,
447+
stream: NowExecDataStreamKind::Stderr,
448+
last: false,
449+
data: stderr_buffer[..bytes_read as usize].to_vec(),
450+
})?;
461451

462452
// Schedule next overlapped read
463453
// SAFETY: pipe is valid to read from, as long as it is not closed.
@@ -592,7 +582,8 @@ impl WinApiProcessBuilder {
592582
// Create channel for `task` -> `Process IO thread` communication
593583
let (input_event_tx, input_event_rx) = winapi_signaled_mpsc_channel()?;
594584

595-
let io_notification_tx = io_notification_tx.expect("BUG: io_notification_tx must be Some for non-detached mode");
585+
let io_notification_tx =
586+
io_notification_tx.expect("BUG: io_notification_tx must be Some for non-detached mode");
596587

597588
let join_handle = std::thread::spawn(move || {
598589
let run_result = if io_redirection {
@@ -654,14 +645,8 @@ fn prepare_process(
654645

655646
let environment_block = (!env.is_empty()).then(|| make_environment_block(env)).transpose()?;
656647

657-
// Control console window visibility:
658-
// - CREATE_NEW_CONSOLE creates a new console window
659-
// - SW_HIDE hides the console window
660648
let mut creation_flags = NORMAL_PRIORITY_CLASS | CREATE_NEW_PROCESS_GROUP | CREATE_NEW_CONSOLE;
661649

662-
startup_info.dwFlags |= STARTF_USESHOWWINDOW;
663-
startup_info.wShowWindow = u16::try_from(SW_HIDE.0).expect("SHOW_WINDOW_CMD fits into u16");
664-
665650
if environment_block.is_some() {
666651
creation_flags |= CREATE_UNICODE_ENVIRONMENT;
667652
}

‎devolutions-session/src/dvc/task.rs‎

Lines changed: 14 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -316,6 +316,16 @@ impl MessageProcessor {
316316
Ok(())
317317
}
318318

319+
async fn send_detached_process_success(&self, session_id: u32) -> Result<(), ExecError> {
320+
self.io_notification_tx
321+
.send(ServerChannelEvent::SessionExited {
322+
session_id,
323+
exit_code: 0,
324+
})
325+
.await?;
326+
Ok(())
327+
}
328+
319329
pub(crate) async fn process_message(
320330
&mut self,
321331
message: NowMessage<'static>,
@@ -570,15 +580,7 @@ impl MessageProcessor {
570580
if exec_msg.is_detached() {
571581
// Detached mode: fire-and-forget, no IO redirection
572582
run_process.run_detached(exec_msg.session_id())?;
573-
574-
// Send success immediately for detached processes
575-
self.io_notification_tx
576-
.send(ServerChannelEvent::SessionExited {
577-
session_id: exec_msg.session_id(),
578-
exit_code: 0,
579-
})
580-
.await?;
581-
583+
self.send_detached_process_success(exec_msg.session_id()).await?;
582584
return Ok(());
583585
}
584586

@@ -612,15 +614,7 @@ impl MessageProcessor {
612614
if batch_msg.is_detached() {
613615
// Detached mode: fire-and-forget, no IO redirection
614616
run_batch.run_detached(batch_msg.session_id())?;
615-
616-
// Send success immediately for detached processes
617-
self.io_notification_tx
618-
.send(ServerChannelEvent::SessionExited {
619-
session_id: batch_msg.session_id(),
620-
exit_code: 0,
621-
})
622-
.await?;
623-
617+
self.send_detached_process_success(batch_msg.session_id()).await?;
624618
return Ok(());
625619
}
626620

@@ -671,15 +665,7 @@ impl MessageProcessor {
671665
if winps_msg.is_detached() {
672666
// Detached mode: fire-and-forget, no IO redirection
673667
run_process.run_detached(winps_msg.session_id())?;
674-
675-
// Send success immediately for detached processes
676-
self.io_notification_tx
677-
.send(ServerChannelEvent::SessionExited {
678-
session_id: winps_msg.session_id(),
679-
exit_code: 0,
680-
})
681-
.await?;
682-
668+
self.send_detached_process_success(winps_msg.session_id()).await?;
683669
return Ok(());
684670
}
685671

@@ -732,15 +718,7 @@ impl MessageProcessor {
732718
if winps_msg.is_detached() {
733719
// Detached mode: fire-and-forget, no IO redirection
734720
run_process.run_detached(winps_msg.session_id())?;
735-
736-
// Send success immediately for detached processes
737-
self.io_notification_tx
738-
.send(ServerChannelEvent::SessionExited {
739-
session_id: winps_msg.session_id(),
740-
exit_code: 0,
741-
})
742-
.await?;
743-
721+
self.send_detached_process_success(winps_msg.session_id()).await?;
744722
return Ok(());
745723
}
746724

0 commit comments

Comments
 (0)