mirror of
https://github.com/gesellix/Bose-SoundTouch.git
synced 2026-08-19 00:56:16 +00:00
fix(ssh): reuse one SSH connection across RevertMigration's ~17 calls
Confirmed on real hardware (192.168.178.28): RevertMigration's full call graph (revertXMLConfig/revertHosts/revertResolvConf/revertAftertouchHook/ removeRcLocalHooks/revertCACert) makes 17 separate client.Run() calls, and pkg/ssh.Client.Run/UploadContent each dialed a brand-new SSH connection per call with no reuse. Hitting a resource-constrained speaker with 17 rapid reconnects overwhelmed it -- confirmed via a follow-up plain SSH command timing out at the TCP level, and the speaker going visibly unresponsive. Gives pkg/ssh.Client an opt-in persistent connection: Connect() dials once and caches it, Close() releases it, and a shared dial() helper makes Run/UploadContent reuse the cached connection when one's open, falling back to today's per-call dial otherwise. RevertMigration now calls Connect() once and defer Close(), collapsing 17 connections into 1. The other ~21 m.NewSSH() call sites in pkg/service/setup never call Connect, so their behavior is completely unchanged -- this only touches the one function that was actually causing real-world problems. SSHClient interface gained Connect()/Close(); both test mocks (pkg/service/setup/setup_test.go, pkg/service/handlers/handlers_setup_test.go) got no-op stubs. Added TestClose_NoOpWithoutConnect and TestConnect_DialFailureLeavesConnNil in pkg/ssh/ssh_test.go -- these don't prove connection reuse against a real server (Client.Run hardcodes :22, no configurable port for a test listener), so that specific behavior is verified by code review (a single `if c.conn != nil` branch) plus the real-hardware confirmation above, not an automated integration test. Also fixes the web UI's "Revert to Defaults" button, which calls the same RevertMigration code path.
This commit is contained in:
@@ -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 }
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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 {
|
||||
|
||||
+69
-12
@@ -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)
|
||||
}
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user