From e4eb96e908b57dcbc423a40ced3c82b469696ccc Mon Sep 17 00:00:00 2001 From: dawn Date: Tue, 25 Aug 2026 00:23:00 +0900 Subject: [PATCH] shuttle/src/pty,spindle/engines/microvm: make debug shell startup failures visible Signed-off-by: dawn --- shuttle/src/pty.rs | 59 +++++++++++++++++++++-- spindle/engines/microvm/debug.go | 22 +++++++-- spindle/engines/microvm/debug_e2e_test.go | 33 +++++++++++-- spindle/engines/microvm/debugssh.go | 42 ++++++++++++++-- 4 files changed, 139 insertions(+), 17 deletions(-) diff --git a/shuttle/src/pty.rs b/shuttle/src/pty.rs index 2ae4d811c..ed7fdba2b 100644 --- a/shuttle/src/pty.rs +++ b/shuttle/src/pty.rs @@ -18,7 +18,7 @@ pub async fn run(host_cid: u32, open: v1::OpenDebugShell) { } async fn serve(host_cid: u32, open: v1::OpenDebugShell) -> Result<()> { - let conn = VsockStream::connect(VsockAddr::new(host_cid, open.vsock_port)) + let mut conn = VsockStream::connect(VsockAddr::new(host_cid, open.vsock_port)) .await .with_context(|| format!("dial host debug vsock port {}", open.vsock_port))?; info!(port = open.vsock_port, "debug shell connected"); @@ -26,10 +26,15 @@ async fn serve(host_cid: u32, open: v1::OpenDebugShell) -> Result<()> { let rows = clamp_tty_dim(open.rows); let cols = clamp_tty_dim(open.cols); - let spec = debug_shell_spec(&open)?; - - let (mut pty_reader, mut pty_writer, mut child) = - command::spawn_pty(spec, rows, cols).context("spawn pty shell")?; + let (mut pty_reader, mut pty_writer, mut child) = match debug_shell_spec(&open) + .and_then(|spec| command::spawn_pty(spec, rows, cols).context("spawn pty shell")) + { + Ok(child) => child, + Err(error) => { + report_start_failure(&mut conn, &error).await; + return Err(error); + } + }; let pid = child.id(); let (conn_reader, conn_writer) = tokio::io::split(conn); @@ -119,6 +124,34 @@ async fn serve(host_cid: u32, open: v1::OpenDebugShell) -> Result<()> { info!(exit_code, "debug shell session ended"); Ok(()) } +async fn report_start_failure(conn: &mut VsockStream, error: &anyhow::Error) { + let detail = format!("{error:#}"); + let (output, exit) = start_failure_messages(&detail); + let _ = protocol::write_message(conn, &output).await; + let _ = protocol::write_message(conn, &exit).await; +} + +fn start_failure_messages(detail: &str) -> (Message, Message) { + let output = Message { + id: "pty".to_owned(), + pty_data: Some(v1::PtyData { + data: format!("error: debug shell failed: {detail}\r\n") + .into_bytes() + .into(), + }), + ..Default::default() + }; + let exit = Message { + id: "pty".to_owned(), + exec_exit: Some(v1::ExecExit { + exit_code: 1, + error: detail.to_owned(), + timed_out: false, + }), + ..Default::default() + }; + (output, exit) +} fn debug_shell_spec(open: &v1::OpenDebugShell) -> Result { let failed_step = open @@ -162,6 +195,22 @@ mod tests { use super::*; + #[test] + fn debug_shell_start_failure_is_sent_to_client() { + let detail = "debug shell request is missing failed step context"; + let (output, exit) = start_failure_messages(detail); + + let output = output.pty_data.unwrap(); + assert_eq!( + output.data.as_ref(), + b"error: debug shell failed: debug shell request is missing failed step context\r\n" + ); + + let exit = exit.exec_exit.unwrap(); + assert_eq!(exit.exit_code, 1); + assert_eq!(exit.error, detail); + } + #[test] fn debug_shell_uses_failed_step_environment_and_devshell() { let open = v1::OpenDebugShell { diff --git a/spindle/engines/microvm/debug.go b/spindle/engines/microvm/debug.go index 28d5ddda9..0c536e596 100644 --- a/spindle/engines/microvm/debug.go +++ b/spindle/engines/microvm/debug.go @@ -219,8 +219,15 @@ type DebugSession struct { out chan []byte leftover []byte - exitCode int closeOne sync.Once + + stateMu sync.Mutex + state debugSessionResult +} + +type debugSessionResult struct { + exit *agentv1.ExecExit + readError string } func newDebugSession(conn net.Conn, ln net.Listener, l *slog.Logger) *DebugSession { @@ -243,13 +250,18 @@ func (d *DebugSession) readLoop() { if err != nil { if !errors.Is(err, io.EOF) { d.l.Debug("debug shell decode ended", "error", err) + d.stateMu.Lock() + d.state.readError = err.Error() + d.stateMu.Unlock() } return } if p := msg.PtyData; p != nil && len(p.Data) > 0 { d.out <- p.Data } else if p := msg.ExecExit; p != nil { - d.exitCode = int(p.ExitCode) + d.stateMu.Lock() + d.state.exit = p + d.stateMu.Unlock() return } } @@ -285,7 +297,11 @@ func (d *DebugSession) Resize(rows, cols int) error { }) } -func (d *DebugSession) ExitCode() int { return d.exitCode } +func (d *DebugSession) result() debugSessionResult { + d.stateMu.Lock() + defer d.stateMu.Unlock() + return d.state +} func (d *DebugSession) Close() error { var err error diff --git a/spindle/engines/microvm/debug_e2e_test.go b/spindle/engines/microvm/debug_e2e_test.go index 2a5717fc4..6d4136241 100644 --- a/spindle/engines/microvm/debug_e2e_test.go +++ b/spindle/engines/microvm/debug_e2e_test.go @@ -117,11 +117,10 @@ func TestDebugShellStaysOpenWhileIdleE2E(t *testing.T) { } engine := &Engine{l: logger, debug: make(map[string]debugTarget)} engine.registerDebugTarget(wid, debugTarget{ - cid: vm.CID(), - agent: agent, - failedStep: failedStep, - connected: make(chan struct{}), - released: make(chan struct{}), + cid: vm.CID(), + agent: agent, + connected: make(chan struct{}), + released: make(chan struct{}), }) handle := newDebugHandle(wid) @@ -178,6 +177,30 @@ func TestDebugShellStaysOpenWhileIdleE2E(t *testing.T) { "-i", clientKeyPath, "-p", port, } + missingContextArgs := append([]string{"-S", "none", "-tt"}, commonArgs...) + missingContextArgs = append(missingContextArgs, target) + missingContextOutput, err := exec.CommandContext(ctx, "ssh", missingContextArgs...).CombinedOutput() + if err == nil { + t.Fatalf("debug shell without failed step context exited successfully\noutput:\n%s", missingContextOutput) + } + wantMissingContext := "error: debug shell failed: debug shell request is missing failed step context" + if !strings.Contains(string(missingContextOutput), wantMissingContext) { + t.Fatalf( + "missing-context error did not reach SSH client\nwant: %s\noutput:\n%s", + wantMissingContext, + missingContextOutput, + ) + } + if _, ok := engine.lookupDebugTarget(handle); ok { + t.Fatal("debug target remained registered after missing-context failure") + } + engine.registerDebugTarget(wid, debugTarget{ + cid: vm.CID(), + agent: agent, + failedStep: failedStep, + connected: make(chan struct{}), + released: make(chan struct{}), + }) masterArgs := append([]string{"-M", "-N", "-S", controlPath}, commonArgs...) masterArgs = append(masterArgs, target) master := exec.CommandContext(ctx, "ssh", masterArgs...) diff --git a/spindle/engines/microvm/debugssh.go b/spindle/engines/microvm/debugssh.go index 1f9bd7740..31c4b0033 100644 --- a/spindle/engines/microvm/debugssh.go +++ b/spindle/engines/microvm/debugssh.go @@ -119,12 +119,46 @@ func (e *Engine) debugHandle(sess ssh.Session) { }() // keyboard -> shell, runs until the client hangs up - go func() { _, _ = io.Copy(debug, sess) }() + go func() { + if _, err := io.Copy(debug, sess); err != nil { + l.Debug("debug ssh input copy ended", "error", err) + } + }() // shell -> client, returns when the shell exits (Read hits EOF) - _, _ = io.Copy(sess, debug) + _, outputErr := io.Copy(sess, debug) + + result := debug.result() + code := 0 + guestError := "" + guestReportedExit := result.exit != nil + if guestReportedExit { + code = int(result.exit.ExitCode) + guestError = result.exit.Error + } + if guestError != "" && code == 0 { + code = 1 + } + if !guestReportedExit { + code = 1 + detail := "debug shell channel closed before the guest reported an exit" + if result.readError != "" { + detail += ": " + result.readError + } + fmt.Fprintf(sess.Stderr(), "error: %s\n", detail) + } + if outputErr != nil { + code = 1 + fmt.Fprintf(sess.Stderr(), "error: debug shell SSH output failed: %v\n", outputErr) + } - code := debug.ExitCode() - l.Info("debug shell closed", "exitCode", code) + l.Info( + "debug shell closed", + "exitCode", code, + "guestReportedExit", guestReportedExit, + "guestError", guestError, + "debugReadError", result.readError, + "sshOutputError", outputErr, + ) // the user is done; let retention tear the VM down now instead of waiting // out the rest of the grace period e.releaseDebugTarget(jobID) -- 2.51.2