fix(core): do not set tasks which cannot be interactive to interactive (nrwl#31240)

<!-- Please make sure you have read the submission guidelines before
posting an PR -->
<!--
https://github.com/nrwl/nx/blob/master/CONTRIBUTING.md#-submitting-a-pr
-->

<!-- Please make sure that your commit message follows our format -->
<!-- Example: `fix(nx): must begin with lowercase` -->

<!-- If this is a particularly complex change or feature addition, you
can request a dedicated Nx release for this pull request branch. Mention
someone from the Nx team or the `@nrwl/nx-pipelines-reviewers` and they
will confirm if the PR warrants its own release for testing purposes,
and generate it for you if appropriate. -->

## Current Behavior
<!-- This is the behavior we have today -->

Some tasks cannot be interactive because of the way they are
implemented. At the moment, those tasks can be set to interactive mode.

## Expected Behavior
<!-- This is the behavior we should expect with the changes in this PR
-->

Tasks which cannot be interactive are not set as interactive

## Related Issue(s)
<!-- Please link the issue being fixed so it gets closed when this is
merged. -->

Fixes #

Author: FrozenPandaz

packages/nx/src/native/pseudo_terminal/child_process.rs

@@ -9,8 +9,9 @@ use napi::{
         ErrorStrategy::Fatal, ThreadsafeFunction, ThreadsafeFunctionCallMode::NonBlocking,
     },
 };
+use parking_lot::Mutex;
 use std::io::Write;
-use std::sync::{Arc, Mutex, RwLock};
+use std::sync::{Arc, RwLock};
 use tracing::warn;
 use vt100_ctt::Parser;
 

packages/nx/src/native/pseudo_terminal/command/unix.rs

@@ -46,10 +46,9 @@ pub fn write_to_pty(stdin: &mut Stdin, writer: WriterArc) -> anyhow::Result<()>
                 mio::Token(0) => {
                     // Read data from stdin
                     loop {
-                        let mut writer = writer.lock().expect("Failed to lock writer");
-
                         match stdin.read(&mut buffer) {
                             Ok(n) => {
+                                let mut writer = writer.lock();
                                 writer.write_all(&buffer[..n])?;
                                 writer.flush()?;
                             }

packages/nx/src/native/pseudo_terminal/command/windows.rs

@@ -26,7 +26,7 @@ pub fn handle_path_space(path: String) -> String {
 }
 
 pub fn write_to_pty(stdin: &mut Stdin, writer: WriterArc) -> anyhow::Result<()> {
-    let mut writer = writer.lock().expect("Failed to lock writer");
+    let mut writer = writer.lock();
     std::io::copy(stdin, writer.as_mut())
         .map_err(|e| anyhow::anyhow!(e))
         .map(|_| ())

packages/nx/src/native/pseudo_terminal/process_killer/unix.rs

@@ -2,6 +2,7 @@ use nix::{
     sys::signal::{Signal as NixSignal, kill},
     unistd::Pid,
 };
+use tracing::debug;
 
 pub struct ProcessKiller {
     pid: i32,
@@ -13,12 +14,12 @@ impl ProcessKiller {
     }
 
     pub fn kill(&self, signal: Option<&str>) -> anyhow::Result<()> {
+        let signal = signal.unwrap_or("SIGINT");
+        debug!("Killing process {} with {}", &self.pid, signal);
         let pid = Pid::from_raw(self.pid);
         match kill(
             pid,
-            NixSignal::from(
-                Signal::try_from(signal.unwrap_or("SIGINT")).map_err(|e| anyhow::anyhow!(e))?,
-            ),
+            NixSignal::from(Signal::try_from(signal).map_err(|e| anyhow::anyhow!(e))?),
         ) {
             Ok(_) => Ok(()),
             Err(e) => Err(anyhow::anyhow!("Failed to kill process: {}", e)),

packages/nx/src/native/pseudo_terminal/pseudo_terminal.rs

@@ -6,9 +6,10 @@ use crossterm::{
     tty::IsTty,
 };
 use napi::bindgen_prelude::*;
+use parking_lot::Mutex;
 use portable_pty::{CommandBuilder, NativePtySystem, PtyPair, PtySize, PtySystem};
 use std::io::stdout;
-use std::sync::{Mutex, RwLock};
+use std::sync::RwLock;
 use std::{
     collections::HashMap,
     io::{Read, Write},

packages/nx/src/native/tui/app.rs

@@ -9,13 +9,12 @@ use ratatui::style::Style;
 use ratatui::text::{Line, Span};
 use ratatui::widgets::{Block, Borders, Paragraph};
 use std::collections::HashMap;
-use std::io::{self, Write};
-use std::sync::{Arc, Mutex, RwLock};
+use std::io;
+use std::sync::{Arc, Mutex};
 use tokio::sync::mpsc;
 use tokio::sync::mpsc::UnboundedSender;
 use tracing::{debug, trace};
 use tui_logger::{LevelFilter, TuiLoggerSmartWidget, TuiWidgetEvent, TuiWidgetState};
-use vt100_ctt::Parser;
 
 use crate::native::tui::tui::Tui;
 use crate::native::{
@@ -174,22 +173,30 @@ impl App {
 
     pub fn update_task_status(&mut self, task_id: String, status: TaskStatus) {
         self.dispatch_action(Action::UpdateTaskStatus(task_id.clone(), status));
-        if status == TaskStatus::InProgress && self.tasks.len() == 1 {
+        if status == TaskStatus::InProgress && self.should_set_interactive_by_default(&task_id) {
             self.terminal_pane_data[0].set_interactive(true);
         }
     }
 
+    fn should_set_interactive_by_default(&self, task_id: &str) -> bool {
+        self.tasks.len() == 1
+            && self
+                .pty_instances
+                .get(task_id)
+                .is_some_and(|pty| pty.can_be_interactive())
+    }
+
     pub fn print_task_terminal_output(&mut self, task_id: String, output: String) {
         // Tasks run within a pseudo-terminal always have a pty instance and do not need a new one
         // Tasks not run within a pseudo-terminal need a new pty instance to print output
         if !self.pty_instances.contains_key(&task_id) {
-            let (parser, parser_and_writer) = Self::create_empty_parser_and_noop_writer();
+            let pty = PtyInstance::non_interactive();
 
             // Add ANSI escape sequence to hide cursor at the end of output, it would be confusing to have it visible when a task is a cache hit
             let output_with_hidden_cursor = format!("{}\x1b[?25l", output);
-            Self::write_output_to_parser(parser, output_with_hidden_cursor);
+            Self::write_output_to_parser(&pty, output_with_hidden_cursor);
 
-            self.create_and_register_pty_instance(&task_id, parser_and_writer);
+            self.register_pty_instance(&task_id, pty);
             // Ensure the pty instances get resized appropriately
             let _ = self.debounce_pty_resize();
             return;
@@ -255,23 +262,31 @@ impl App {
     }
 
     // A pseudo-terminal running task will provide the parser and writer directly
-    pub fn register_running_task(
+    pub fn register_running_interactive_task(
         &mut self,
         task_id: String,
         parser_and_writer: External<(ParserArc, WriterArc)>,
     ) {
-        self.create_and_register_pty_instance(&task_id, parser_and_writer);
-        self.update_task_status(task_id.clone(), TaskStatus::InProgress);
+        debug!("Registering interactive task: {task_id}");
+        let pty =
+            PtyInstance::interactive(parser_and_writer.0.clone(), parser_and_writer.1.clone());
+        self.register_running_task(task_id, pty)
     }
 
-    pub fn register_running_task_with_empty_parser(&mut self, task_id: String) {
-        let (_, parser_and_writer) = Self::create_empty_parser_and_noop_writer();
-        self.create_and_register_pty_instance(&task_id, parser_and_writer);
+    pub fn register_running_non_interactive_task(&mut self, task_id: String) {
+        debug!("Registering non-interactive task: {task_id}");
+        let pty = PtyInstance::non_interactive();
+        self.register_pty_instance(&task_id, pty);
         self.update_task_status(task_id.clone(), TaskStatus::InProgress);
         // Ensure the pty instances get resized appropriately
         let _ = self.debounce_pty_resize();
     }
 
+    fn register_running_task(&mut self, task_id: String, pty: PtyInstance) {
+        self.register_pty_instance(&task_id, pty);
+        self.update_task_status(task_id.clone(), TaskStatus::InProgress);
+    }
+
     pub fn append_task_output(&mut self, task_id: String, output: String) {
         let pty = self
             .pty_instances
@@ -973,13 +988,14 @@ impl App {
                                 let terminal_pane_data = &mut self.terminal_pane_data[pane_idx];
                                 terminal_pane_data.is_continuous = task.continuous;
                                 let in_progress = task.status == TaskStatus::InProgress;
-                                terminal_pane_data.can_be_interactive = in_progress;
                                 if !in_progress {
                                     terminal_pane_data.set_interactive(false);
                                 }
 
                                 let mut has_pty = false;
                                 if let Some(pty) = self.pty_instances.get(&relevant_pane_task) {
+                                    terminal_pane_data.can_be_interactive =
+                                        in_progress && pty.can_be_interactive();
                                     terminal_pane_data.pty = Some(pty.clone());
                                     has_pty = true;
                                 }
@@ -1487,41 +1503,17 @@ impl App {
         self.layout_manager.get_task_list_visibility() == TaskListVisibility::Hidden
     }
 
-    fn create_and_register_pty_instance(
-        &mut self,
-        task_id: &str,
-        parser_and_writer: External<(ParserArc, WriterArc)>,
-    ) {
+    fn register_pty_instance(&mut self, task_id: &str, pty: PtyInstance) {
         // Access the contents of the External
-        let pty = Arc::new(
-            PtyInstance::new(
-                task_id.to_string(),
-                parser_and_writer.0.clone(),
-                parser_and_writer.1.clone(),
-            )
-            .map_err(|e| napi::Error::from_reason(format!("Failed to create PTY: {}", e)))
-            .unwrap(),
-        );
+        let pty = Arc::new(pty);
 
         self.pty_instances.insert(task_id.to_string(), pty);
     }
 
-    fn create_empty_parser_and_noop_writer() -> (ParserArc, External<(ParserArc, WriterArc)>) {
-        // Use sane defaults for rows, cols and scrollback buffer size. The dimensions will be adjusted dynamically later.
-        let parser = Arc::new(RwLock::new(Parser::new(24, 80, 10000)));
-        let writer: Arc<Mutex<Box<dyn Write + Send>>> =
-            Arc::new(Mutex::new(Box::new(std::io::sink())));
-        (parser.clone(), External::new((parser, writer)))
-    }
-
     // Writes the given output to the given parser, used for the case where a task is a cache hit, or when it is run outside of the rust pseudo-terminal
-    fn write_output_to_parser(parser: ParserArc, output: String) {
+    fn write_output_to_parser(parser: &PtyInstance, output: String) {
         let normalized_output = normalize_newlines(output.as_bytes());
-        parser
-            .write()
-            .unwrap()
-            .write_all(&normalized_output)
-            .unwrap();
+        parser.process_output(&normalized_output);
     }
 
     fn display_and_focus_current_task_in_terminal_pane(&mut self, force_spacebar_mode: bool) {

packages/nx/src/native/tui/components/terminal_pane.rs

@@ -11,6 +11,7 @@ use ratatui::{
     },
 };
 use std::{io, sync::Arc};
+use tracing::debug;
 use tui_term::widget::PseudoTerminal;
 
 use crate::native::tui::components::tasks_list::TaskStatus;
@@ -100,6 +101,7 @@ impl TerminalPaneData {
                 _ if self.is_interactive => match key.code {
                     KeyCode::Char(c) if key.modifiers.contains(KeyModifiers::CONTROL) => {
                         let ascii_code = (c as u8) - 0x60;
+                        debug!("Sending ASCII code: {}", &[ascii_code].escape_ascii());
                         pty_mut.write_input(&[ascii_code])?;
                     }
                     KeyCode::Char(c) => {

packages/nx/src/native/tui/lifecycle.rs

@@ -256,14 +256,14 @@ impl AppLifeCycle {
     ) {
         self.app
             .lock()
-            .register_running_task(task_id, parser_and_writer)
+            .register_running_interactive_task(task_id, parser_and_writer)
     }
 
     #[napi]
     pub fn register_running_task_with_empty_parser(&mut self, task_id: String) {
         self.app
             .lock()
-            .register_running_task_with_empty_parser(task_id)
+            .register_running_non_interactive_task(task_id)
     }
 
     #[napi]

packages/nx/src/native/tui/pty.rs

@@ -1,8 +1,9 @@
 use super::utils::normalize_newlines;
 use crossterm::event::{KeyCode, KeyEvent, MouseEvent, MouseEventKind};
+use parking_lot::Mutex;
 use std::{
     io::{self, Write},
-    sync::{Arc, Mutex, RwLock},
+    sync::{Arc, RwLock},
 };
 use tracing::debug;
 use vt100_ctt::Parser;
@@ -29,28 +30,42 @@ impl std::ops::Deref for PtyScreenRef<'_> {
 
 #[derive(Clone)]
 pub struct PtyInstance {
-    pub task_id: String,
-    pub parser: Arc<RwLock<Parser>>,
-    pub writer: Arc<Mutex<Box<dyn Write + Send>>>,
+    parser: Arc<RwLock<Parser>>,
+    writer: Option<Arc<Mutex<Box<dyn Write + Send>>>>,
     rows: u16,
     cols: u16,
 }
 
 impl PtyInstance {
-    pub fn new(
-        task_id: String,
+    pub fn interactive(
         parser: Arc<RwLock<Parser>>,
         writer: Arc<Mutex<Box<dyn Write + Send>>>,
-    ) -> io::Result<Self> {
+    ) -> Self {
         // Read the dimensions from the parser
         let (rows, cols) = parser.read().unwrap().screen().size();
-        Ok(Self {
-            task_id,
+        Self {
             parser,
-            writer,
+            writer: Some(writer),
             rows,
             cols,
-        })
+        }
+    }
+
+    pub fn non_interactive() -> Self {
+        // Use sane defaults for rows, cols and scrollback buffer size. The dimensions will be adjusted dynamically later.
+        let rows = 24;
+        let cols = 80;
+        let parser = Arc::new(RwLock::new(Parser::new(rows, cols, 10000)));
+        Self {
+            parser,
+            writer: None,
+            rows,
+            cols,
+        }
+    }
+
+    pub fn can_be_interactive(&self) -> bool {
+        self.writer.is_some()
     }
 
     pub fn resize(&mut self, rows: u16, cols: u16) -> io::Result<()> {
@@ -86,9 +101,13 @@ impl PtyInstance {
     }
 
     pub fn write_input(&mut self, input: &[u8]) -> io::Result<()> {
-        if let Ok(mut writer_guard) = self.writer.lock() {
+        if let Some(writer) = &self.writer {
+            let mut writer_guard = writer.lock();
+            debug!("Writing input: {:?}", input);
             writer_guard.write_all(input)?;
             writer_guard.flush()?;
+        } else {
+            debug!("Swallowing input: {:?}", input);
         }
 
         Ok(())