Improve error handling, config safety, and installer reliability

.goreleaser.yaml

@@ -72,7 +72,6 @@ changelog:
     exclude:
       - "^docs:"
       - "^test:"
-      - "^chore:"
       - "Merge pull request"
       - "Merge branch"
 

install.sh

@@ -27,7 +27,7 @@ warn() {
 }
 
 error() {
-    printf "${RED}[ERROR]${NC} %s\n" "$1"
+    printf "${RED}[ERROR]${NC} %s\n" "$1" >&2
     exit 1
 }
 
@@ -63,7 +63,7 @@ verify_checksum() {
 
     info "Verifying checksum..."
 
-    CHECKSUMS=$(curl -sSL "${CHECKSUM_URL}") || {
+    CHECKSUMS=$(curl -fsSL "${CHECKSUM_URL}") || {
         warn "Could not download checksums, skipping verification"
         return 0
     }
@@ -92,8 +92,8 @@ verify_checksum() {
 
 # Download and install
 install() {
-    OS=$(detect_os)
-    ARCH=$(detect_arch)
+    OS=$(detect_os) || exit 1
+    ARCH=$(detect_arch) || exit 1
 
     info "Detected OS: ${OS}, Arch: ${ARCH}"
 
@@ -120,10 +120,10 @@ install() {
 
     # Create temp directory
     TMP_DIR=$(mktemp -d)
-    trap "rm -rf ${TMP_DIR}" EXIT
+    trap "rm -rf ${TMP_DIR}" EXIT INT TERM
 
     # Download
-    curl -sSL "${DOWNLOAD_URL}" -o "${TMP_DIR}/${FILENAME}"
+    curl -fsSL "${DOWNLOAD_URL}" -o "${TMP_DIR}/${FILENAME}"
 
     # Verify checksum
     cd "${TMP_DIR}"
@@ -137,6 +137,15 @@ install() {
     fi
 
     # Install
+    if [ ! -d "${INSTALL_DIR}" ]; then
+        info "Creating ${INSTALL_DIR}"
+        if [ -w "$(dirname "${INSTALL_DIR}")" ]; then
+            mkdir -p "${INSTALL_DIR}"
+        else
+            sudo mkdir -p "${INSTALL_DIR}"
+        fi
+    fi
+
     if [ -w "${INSTALL_DIR}" ]; then
         mv "${BINARY_NAME}" "${INSTALL_DIR}/${BINARY_NAME}"
     else
@@ -152,7 +161,10 @@ install() {
     if "${INSTALL_DIR}/${BINARY_NAME}" --version >/dev/null 2>&1; then
         info "Verified: $(${INSTALL_DIR}/${BINARY_NAME} --version | head -1)"
     else
-        warn "Binary installed but verification failed"
+        warn "Binary installed but could not run it"
+        if [ "$OS" = "linux" ]; then
+            warn "Linux requires libasound2 (ALSA). Install it with: sudo apt-get install libasound2"
+        fi
     fi
 }
 

internal/config/config.go

@@ -4,6 +4,7 @@ import (
 	"fmt"
 	"os"
 	"path/filepath"
+	"sync"
 
 	"github.com/gdamore/tcell/v2"
 	"gopkg.in/yaml.v3"
@@ -65,6 +66,8 @@ type Config struct {
 	Autostart   bool     `yaml:"autostart"`
 	Favorites   []string `yaml:"favorites"`
 	Theme       Theme    `yaml:"theme"`
+
+	saveMu sync.Mutex `yaml:"-"`
 }
 
 func GetConfigPath() (string, error) {
@@ -104,6 +107,9 @@ func Load() (*Config, error) {
 
 // Save writes the configuration to disk atomically using temp file + rename.
 func (c *Config) Save() error {
+	c.saveMu.Lock()
+	defer c.saveMu.Unlock()
+
 	configPath, err := GetConfigPath()
 	if err != nil {
 		return err

internal/player/player.go

@@ -650,13 +650,24 @@ func (p *Player) reconnectWithRotation(s *station.Station, streamURLs []string,
 	return fmt.Errorf("reconnection failed: %w", lastErr)
 }
 
+type httpStatusError struct {
+	StatusCode int
+	Status     string
+}
+
+func (e *httpStatusError) Error() string {
+	return fmt.Sprintf("stream returned status %d: %s", e.StatusCode, e.Status)
+}
+
 func isNonRetryableError(err error) bool {
-	errStr := err.Error()
-	// HTTP errors that won't change with retry on this specific URL
-	return strings.Contains(errStr, "status 401") ||
-		strings.Contains(errStr, "status 403") ||
-		strings.Contains(errStr, "status 404") ||
-		strings.Contains(errStr, "status 410")
+	var statusErr *httpStatusError
+	if errors.As(err, &statusErr) {
+		switch statusErr.StatusCode {
+		case 401, 403, 404, 410:
+			return true
+		}
+	}
+	return false
 }
 
 func (p *Player) playStreamURL(ctx context.Context, s *station.Station, streamURL string) error {
@@ -681,7 +692,7 @@ func (p *Player) playStreamURL(ctx context.Context, s *station.Station, streamUR
 
 	if resp.StatusCode != http.StatusOK {
 		resp.Body.Close()
-		return fmt.Errorf("stream returned status %d: %s", resp.StatusCode, resp.Status)
+		return &httpStatusError{StatusCode: resp.StatusCode, Status: resp.Status}
 	}
 
 	var icyMetaint int
@@ -869,6 +880,15 @@ func (p *Player) readNetworkStream(ctx context.Context, respBody io.ReadCloser,
 				}
 
 				metaLen := int(metaLenByte) * 16
+				if metaLen > 4080 {
+					log.Warn().Int("metaLen", metaLen).Msg("ICY metadata too large, skipping")
+					if _, err := io.CopyN(io.Discard, bufReader, int64(metaLen)); err != nil {
+						if ctx.Err() != nil {
+							return
+						}
+					}
+					continue
+				}
 				if metaLen > 0 {
 					metaData := make([]byte, metaLen)
 					n, err := io.ReadFull(bufReader, metaData)
@@ -1083,12 +1103,12 @@ func parseStreamInfoFromURL(url string) StreamInfo {
 		info.Format = "AAC"
 	}
 
-	bitrates := []int{320, 256, 192, 130, 128, 64, 32}
-	for _, br := range bitrates {
+	// SomaFM URL convention: groovesalad130.pls = MP3 128kbps (130 is an internal ID, not actual bitrate)
+	for _, br := range []int{320, 256, 192, 130, 128, 64, 32} {
 		brStr := fmt.Sprintf("%d", br)
 		if strings.Contains(url, brStr+".pls") || strings.Contains(url, brStr+".") {
 			info.Bitrate = br
-			if br == 130 { // SomaFM uses 130 for 128kbps streams
+			if br == 130 {
 				info.Bitrate = 128
 			}
 			break

internal/player/player_test.go

@@ -73,10 +73,6 @@ func TestPlayerStateString(t *testing.T) {
 func TestNewPlayer(t *testing.T) {
 	p := NewPlayer()
 
-	if p == nil {
-		t.Fatal("NewPlayer returned nil")
-	}
-
 	if p.isPlaying {
 		t.Error("New player should not be playing")
 	}
@@ -88,28 +84,39 @@ func TestNewPlayer(t *testing.T) {
 
 func TestIsNonRetryableError(t *testing.T) {
 	tests := []struct {
+		name     string
 		err      error
 		expected bool
 	}{
-		{errors.New("status 401"), true},
-		{errors.New("status 403"), true},
-		{errors.New("status 404"), true},
-		{errors.New("status 410"), true},
-		{errors.New("status 500"), false},
-		{errors.New("connection refused"), false},
-		{errors.New("timeout"), false},
+		{"401", &httpStatusError{StatusCode: 401, Status: "Unauthorized"}, true},
+		{"403", &httpStatusError{StatusCode: 403, Status: "Forbidden"}, true},
+		{"404", &httpStatusError{StatusCode: 404, Status: "Not Found"}, true},
+		{"410", &httpStatusError{StatusCode: 410, Status: "Gone"}, true},
+		{"500", &httpStatusError{StatusCode: 500, Status: "Internal Server Error"}, false},
+		{"503", &httpStatusError{StatusCode: 503, Status: "Service Unavailable"}, false},
+		{"wrapped 404", fmt.Errorf("stream failed: %w", &httpStatusError{StatusCode: 404}), true},
+		{"generic error", errors.New("connection refused"), false},
+		{"timeout", errors.New("timeout"), false},
 	}
 
 	for _, tt := range tests {
-		t.Run(tt.err.Error(), func(t *testing.T) {
+		t.Run(tt.name, func(t *testing.T) {
 			result := isNonRetryableError(tt.err)
 			if result != tt.expected {
-				t.Errorf("isNonRetryableError(%q) = %v, want %v", tt.err, result, tt.expected)
+				t.Errorf("isNonRetryableError(%v) = %v, want %v", tt.err, result, tt.expected)
 			}
 		})
 	}
 }
 
+func TestHttpStatusErrorMessage(t *testing.T) {
+	err := &httpStatusError{StatusCode: 404, Status: "404 Not Found"}
+	expected := "stream returned status 404: 404 Not Found"
+	if err.Error() != expected {
+		t.Errorf("got %q, want %q", err.Error(), expected)
+	}
+}
+
 func TestParseStreamInfoFromURL(t *testing.T) {
 	tests := []struct {
 		url             string