diff --git a/pkg/service/handlers/handlers_setup_test.go b/pkg/service/handlers/handlers_setup_test.go index 81573a6..698326f 100644 --- a/pkg/service/handlers/handlers_setup_test.go +++ b/pkg/service/handlers/handlers_setup_test.go @@ -965,3 +965,9 @@ func (m *mockSSH) UploadContent(content []byte, remotePath string) error { return nil } + +// Connect/Close are no-ops here — the mock has no real connection to +// reuse, and every test call already goes through Run/UploadContent above +// regardless of whether Connect was called first. +func (m *mockSSH) Connect() error { return nil } +func (m *mockSSH) Close() error { return nil } diff --git a/pkg/service/setup/setup.go b/pkg/service/setup/setup.go index 7bff356..9db109d 100644 --- a/pkg/service/setup/setup.go +++ b/pkg/service/setup/setup.go @@ -124,9 +124,17 @@ type MigrationSummary struct { } // SSHClient defines the interface for SSH operations. +// +// Connect/Close are optional: Run/UploadContent both work standalone +// (dialing their own one-off connection each time, as they always have). +// Call Connect first when making several calls in a row — e.g. +// RevertMigration's ~17 commands — so they reuse one connection instead of +// dialing fresh every time; defer Close to release it afterward. type SSHClient interface { Run(command string) (string, error) UploadContent(content []byte, remotePath string) error + Connect() error + Close() error } // TelnetClient defines the interface for the device's port-17000 diagnostic @@ -1869,6 +1877,18 @@ func (m *Manager) patchUdhcpcScript(client SSHClient, targetScript, hookMarker s // RevertMigration reverts the speaker to its original Bose cloud configuration. func (m *Manager) RevertMigration(deviceIP string) (string, error) { client := m.NewSSH(deviceIP) + + // This function alone makes ~17 client.Run/UploadContent calls across + // its sub-steps below. Dialing a fresh SSH connection per call (the + // default when Connect isn't used) was confirmed on real hardware to + // overwhelm a resource-constrained speaker; Connect+defer Close keeps + // it to one connection for the whole revert instead. + if err := client.Connect(); err != nil { + return "", fmt.Errorf("failed to connect for revert: %w", err) + } + + defer func() { _ = client.Close() }() + rwCmd := "(rw || mount -o remount,rw /)" var logs string diff --git a/pkg/service/setup/setup_test.go b/pkg/service/setup/setup_test.go index 10d3d57..68ff496 100644 --- a/pkg/service/setup/setup_test.go +++ b/pkg/service/setup/setup_test.go @@ -132,6 +132,12 @@ func (m *mockSSH) UploadContent(content []byte, remotePath string) error { return nil } +// Connect/Close are no-ops here — the mock has no real connection to +// reuse, and every test call already goes through Run/UploadContent above +// regardless of whether Connect was called first. +func (m *mockSSH) Connect() error { return nil } +func (m *mockSSH) Close() error { return nil } + func TestMigrateViaHosts(t *testing.T) { tempDir, err := os.MkdirTemp("", "setup-test") if err != nil { diff --git a/pkg/ssh/ssh.go b/pkg/ssh/ssh.go index 4fe2be3..c70bd81 100644 --- a/pkg/ssh/ssh.go +++ b/pkg/ssh/ssh.go @@ -14,6 +14,15 @@ import ( type Client struct { Host string User string + + // conn is non-nil once Connect has been called, and is then reused by + // Run/UploadContent until Close. Left nil, each Run/UploadContent call + // dials its own one-off connection as before — Connect is opt-in for + // callers making several calls in a row (e.g. RevertMigration's ~17 + // commands), where dialing fresh every time is both slow and, on a + // resource-constrained speaker, has been observed to overwhelm the + // device (#614 self-test, 2026-08-16). + conn *ssh.Client } // NewClient creates a new SSH client for the given host. The default user is "root". @@ -66,21 +75,71 @@ func (c *Client) getConfig() *ssh.ClientConfig { } } +// Connect opens a persistent SSH connection reused by subsequent +// Run/UploadContent calls, instead of each dialing its own. Call Close when +// done with it. Idempotent — calling Connect again while already connected +// is a no-op. Skip this for a single (or a rare few) command — dialing +// once and reusing it is only worth the extra Close bookkeeping when +// several calls follow in quick succession. +func (c *Client) Connect() error { + if c.conn != nil { + return nil + } + + conn, err := ssh.Dial("tcp", c.Host+":22", c.getConfig()) + if err != nil { + return fmt.Errorf("failed to dial: %w", err) + } + + c.conn = conn + + return nil +} + +// Close closes the persistent connection opened by Connect, if any. Safe +// to call even when Connect was never called (e.g. every Run/UploadContent +// call so far used its own one-off connection). +func (c *Client) Close() error { + if c.conn == nil { + return nil + } + + err := c.conn.Close() + c.conn = nil + + return err +} + +// dial returns the persistent connection from Connect if one is open, +// otherwise dials a fresh one-off connection for the caller to close via +// the returned closeFunc (a no-op when reusing the persistent connection — +// that one is only closed by an explicit Close call). +func (c *Client) dial() (conn *ssh.Client, closeFunc func(), err error) { + if c.conn != nil { + return c.conn, func() {}, nil + } + + conn, err = ssh.Dial("tcp", c.Host+":22", c.getConfig()) + if err != nil { + return nil, nil, fmt.Errorf("failed to dial: %w", err) + } + + return conn, func() { _ = conn.Close() }, nil +} + // Run executes a command on the remote host and returns the combined stdout and stderr. // // command MUST be a hardcoded shell literal or constructed entirely from // internal, service-controlled values — never from user-supplied HTTP input. func (c *Client) Run(command string) (string, error) { - config := c.getConfig() - - client, err := ssh.Dial("tcp", c.Host+":22", config) + conn, closeConn, err := c.dial() if err != nil { - return "", fmt.Errorf("failed to dial: %w", err) + return "", err } - defer func() { _ = client.Close() }() + defer closeConn() - session, err := client.NewSession() + session, err := conn.NewSession() if err != nil { return "", fmt.Errorf("failed to create session: %w", err) } @@ -133,16 +192,14 @@ func (c *Client) ReadDir(remotePath string) (map[string][]byte, error) { // UploadContent uploads the given content to a file on the remote host using stdin piping. func (c *Client) UploadContent(content []byte, remotePath string) error { - config := c.getConfig() - - client, err := ssh.Dial("tcp", c.Host+":22", config) + conn, closeConn, err := c.dial() if err != nil { - return fmt.Errorf("failed to dial: %w", err) + return err } - defer func() { _ = client.Close() }() + defer closeConn() - session, err := client.NewSession() + session, err := conn.NewSession() if err != nil { return fmt.Errorf("failed to create session: %w", err) } diff --git a/pkg/ssh/ssh_test.go b/pkg/ssh/ssh_test.go index d315e82..071bf1e 100644 --- a/pkg/ssh/ssh_test.go +++ b/pkg/ssh/ssh_test.go @@ -44,3 +44,33 @@ func TestRun_DialFailure(t *testing.T) { t.Errorf("Expected 'failed to dial' error, got: %v", err) } } + +func TestClose_NoOpWithoutConnect(t *testing.T) { + client := NewClient("127.0.0.1") + + if err := client.Close(); err != nil { + t.Errorf("Close on a never-connected client should be a no-op, got: %v", err) + } +} + +func TestConnect_DialFailureLeavesConnNil(t *testing.T) { + client := NewClient("127.0.0.1:0") + + err := client.Connect() + if err == nil { + t.Fatal("Expected dial failure, got nil") + } + + if !strings.Contains(err.Error(), "failed to dial") { + t.Errorf("Expected 'failed to dial' error, got: %v", err) + } + + if client.conn != nil { + t.Error("Connect should leave conn nil after a dial failure, so Run/UploadContent still fall back to their own one-off dial") + } + + // Close after a failed Connect should still be a harmless no-op. + if err := client.Close(); err != nil { + t.Errorf("Close after a failed Connect should be a no-op, got: %v", err) + } +}