Fix jpeg capture cancelling context

cameras.go

@@ -1,6 +1,7 @@
 package securityspy
 
 import (
+	"bytes"
 	"context"
 	"encoding/base64"
 	"errors"
@@ -175,66 +176,131 @@ func (c *Camera) PostG711(audio io.ReadCloser) ([]byte, error) {
 // VidOps defines the image size. ops.FPS is ignored.
 // Makes several attempts in case of an error or time out.
 func (c *Camera) GetJPEG(ops *VidOps) (image.Image, error) {
+	data, err := c.fetchJPEGBytes(ops)
+	if err != nil {
+		return nil, err
+	}
+
+	jpgImage, err := jpeg.Decode(bytes.NewReader(data))
+	if err != nil {
+		return nil, fmt.Errorf("decoding jpeg: %w", err)
+	}
+
+	return jpgImage, nil
+}
+
+// SaveJPEG gets a picture from a camera and puts it in a file (path).
+// Fails if the path already exists. VidOps defines the image size; ops.FPS is ignored.
+// Writes the server JPEG bytes directly (no decode/re-encode).
+func (c *Camera) SaveJPEG(ops *VidOps, path string) error {
+	data, err := c.fetchJPEGBytes(ops)
+	if err != nil {
+		return fmt.Errorf("getting jpeg: %w", err)
+	}
+
+	file, err := os.OpenFile(path, os.O_WRONLY|os.O_CREATE|os.O_EXCL, jpegFilePerm) //nolint:gosec // caller-chosen path
+	if err != nil {
+		if errors.Is(err, os.ErrExist) {
+			return ErrPathExists
+		}
+
+		return fmt.Errorf("creating jpeg: %w", err)
+	}
+
+	_, writeErr := file.Write(data)
+	closeErr := file.Close()
+
+	if writeErr != nil {
+		_ = os.Remove(path)
+
+		return fmt.Errorf("writing jpeg: %w", writeErr)
+	}
+
+	if closeErr != nil {
+		_ = os.Remove(path)
+
+		return fmt.Errorf("closing jpeg: %w", closeErr)
+	}
+
+	return nil
+}
+
+// fetchJPEGBytes downloads a still from ++image. The request context stays alive
+// until the body is fully read — canceling earlier truncates the JPEG.
+// Retries use server.Config.JPEGRetries; each attempt is bounded by Timeout.
+func (c *Camera) fetchJPEGBytes(ops *VidOps) ([]byte, error) {
 	if ops == nil {
 		ops = &VidOps{}
 	}
 
 	ops.FPS = -1 // not used for single image
 
+	client := c.streamHTTPClient() // Timeout=0; context bounds the whole fetch
+
 	var lastErr error
 
 	for range c.server.JPEGTries() {
-		ctx, cancel := context.WithTimeout(context.Background(), c.server.TimeoutDur())
-		resp, err := c.server.GetContext(ctx, "++image", c.makeRequestParams(ops))
-
-		cancel()
-
-		if err != nil {
-			lastErr = fmt.Errorf("getting image: %w", err)
+		data, err := c.fetchJPEGBytesOnce(ops, client)
+		if err == nil {
+			return data, nil
+		}
 
-			continue
+		lastErr = err
+		// Offline / missing cameras won't recover within this call.
+		if errors.Is(err, ErrCameraUnavailable) {
+			return nil, err
 		}
+	}
 
-		jpgImage, err := jpeg.Decode(resp.Body)
-		_ = resp.Body.Close()
+	return nil, lastErr
+}
 
-		if err != nil {
-			lastErr = fmt.Errorf("decoding jpeg: %w", err)
+func (c *Camera) fetchJPEGBytesOnce(ops *VidOps, client *http.Client) ([]byte, error) {
+	ctx, cancel := context.WithTimeout(context.Background(), c.server.TimeoutDur())
 
-			continue
-		}
+	resp, err := c.server.GetContextClient(ctx, "++image", c.makeRequestParams(ops), client)
+	if err != nil {
+		cancel()
 
-		return jpgImage, nil
+		return nil, fmt.Errorf("getting image: %w", err)
 	}
 
-	return nil, lastErr
-}
+	if resp.StatusCode == http.StatusNotFound {
+		_ = resp.Body.Close()
 
-// SaveJPEG gets a picture from a camera and puts it in a file (path).
-// The file will be overwritten if it exists.
-// VidOps defines the image size. ops.FPS is ignored.
-func (c *Camera) SaveJPEG(ops *VidOps, path string) error {
-	if _, err := os.Stat(path); !os.IsNotExist(err) {
-		return ErrPathExists
+		cancel()
+
+		return nil, fmt.Errorf("%w: %s", ErrCameraUnavailable, c.Name)
 	}
 
-	jpgImage, err := c.GetJPEG(ops)
-	if err != nil {
-		return fmt.Errorf("getting jpeg: %w", err)
+	if resp.StatusCode != http.StatusOK {
+		preview, _ := io.ReadAll(io.LimitReader(resp.Body, jpegSOIPreviewMax))
+		_ = resp.Body.Close()
+
+		cancel()
+
+		return nil, fmt.Errorf("%w: status %d (%q)", server.ErrCmdNotOK, resp.StatusCode, preview)
 	}
 
-	oFile, err := os.Create(path) //nolint:gosec // we are creating a file in a safe way.
+	data, err := io.ReadAll(resp.Body)
+	_ = resp.Body.Close()
+
+	cancel()
+
 	if err != nil {
-		return fmt.Errorf("os.Create: %w", err)
+		return nil, fmt.Errorf("reading image: %w", err)
 	}
-	defer oFile.Close()
 
-	err = jpeg.Encode(oFile, jpgImage, nil)
-	if err != nil {
-		return fmt.Errorf("encoding jpeg: %w", err)
+	if len(data) < 2 || data[0] != 0xFF || data[1] != 0xD8 {
+		preview := data
+		if len(preview) > jpegSOIPreviewMax {
+			preview = preview[:jpegSOIPreviewMax]
+		}
+
+		return nil, fmt.Errorf("%w: got %q", ErrInvalidJPEG, preview)
 	}
 
-	return nil
+	return data, nil
 }
 
 // ToggleContinuous arms or disarms continuous capture via ++ssControlContinuous.

cameras_internal_test.go

@@ -7,6 +7,8 @@ import (
 	"image/jpeg"
 	"net/http"
 	"net/http/httptest"
+	"os"
+	"sync/atomic"
 	"testing"
 	"time"
 
@@ -15,6 +17,33 @@ import (
 	"golift.io/securityspy/v2/server"
 )
 
+func TestGetJPEGSucceeds(t *testing.T) {
+	t.Parallel()
+
+	var jpegData bytes.Buffer
+
+	img := image.NewRGBA(image.Rect(0, 0, 1, 1))
+	img.Set(0, 0, color.RGBA{R: 255, A: 255})
+	require.NoError(t, jpeg.Encode(&jpegData, img, nil))
+
+	fakeServer := httptest.NewServer(http.HandlerFunc(func(writer http.ResponseWriter, _ *http.Request) {
+		writer.Header().Set("Content-Type", "image/jpeg")
+		_, _ = writer.Write(jpegData.Bytes())
+	}))
+	defer fakeServer.Close()
+
+	srv := NewMust(&server.Config{
+		URL:     fakeServer.URL + "/",
+		Timeout: server.Duration{Duration: time.Second},
+	})
+
+	camera := &Camera{Number: 2, server: srv}
+
+	got, err := camera.GetJPEG(nil)
+	require.NoError(t, err)
+	assert.NotNil(t, got)
+}
+
 func TestGetJPEGRetriesAndSucceeds(t *testing.T) {
 	t.Parallel()
 
@@ -23,7 +52,7 @@ func TestGetJPEGRetriesAndSucceeds(t *testing.T) {
 	const badAttempts = retryAttempts - 1
 
 	var (
-		requests int
+		requests atomic.Int32
 		jpegData bytes.Buffer
 	)
 
@@ -32,8 +61,8 @@ func TestGetJPEGRetriesAndSucceeds(t *testing.T) {
 	require.NoError(t, jpeg.Encode(&jpegData, img, nil))
 
 	fakeServer := httptest.NewServer(http.HandlerFunc(func(writer http.ResponseWriter, _ *http.Request) {
-		requests++
-		if requests <= badAttempts {
+		n := requests.Add(1)
+		if n <= badAttempts {
 			_, _ = writer.Write([]byte("not-a-jpeg"))
 
 			return
@@ -55,7 +84,136 @@ func TestGetJPEGRetriesAndSucceeds(t *testing.T) {
 	got, err := camera.GetJPEG(nil)
 	require.NoError(t, err)
 	assert.NotNil(t, got)
-	assert.Equal(t, retryAttempts, requests)
+	assert.Equal(t, int32(retryAttempts), requests.Load())
+}
+
+func TestGetJPEGRejectsNonJPEG(t *testing.T) {
+	t.Parallel()
+
+	fakeServer := httptest.NewServer(http.HandlerFunc(func(writer http.ResponseWriter, _ *http.Request) {
+		_, _ = writer.Write([]byte("not-a-jpeg"))
+	}))
+	defer fakeServer.Close()
+
+	srv := NewMust(&server.Config{
+		URL:         fakeServer.URL + "/",
+		Timeout:     server.Duration{Duration: time.Second},
+		JPEGRetries: 1,
+	})
+	camera := &Camera{Number: 2, Name: "X", server: srv}
+
+	_, err := camera.GetJPEG(nil)
+	require.Error(t, err)
+	require.ErrorIs(t, err, ErrInvalidJPEG)
+}
+
+func TestGetJPEGNoRetryOn404(t *testing.T) {
+	t.Parallel()
+
+	var requests atomic.Int32
+
+	fakeServer := httptest.NewServer(http.HandlerFunc(func(writer http.ResponseWriter, _ *http.Request) {
+		requests.Add(1)
+		writer.WriteHeader(http.StatusNotFound)
+		_, _ = writer.Write(bytes.Repeat([]byte("x"), 1024))
+	}))
+	defer fakeServer.Close()
+
+	srv := NewMust(&server.Config{
+		URL:         fakeServer.URL + "/",
+		Timeout:     server.Duration{Duration: time.Second},
+		JPEGRetries: 5,
+	})
+	camera := &Camera{Number: 2, Name: "DeadCam", server: srv}
+
+	_, err := camera.GetJPEG(nil)
+	require.ErrorIs(t, err, ErrCameraUnavailable)
+	assert.Equal(t, int32(1), requests.Load())
+}
+
+func TestSaveJPEGNoOverwrite(t *testing.T) {
+	t.Parallel()
+
+	var jpegData bytes.Buffer
+
+	img := image.NewRGBA(image.Rect(0, 0, 1, 1))
+	img.Set(0, 0, color.RGBA{R: 255, A: 255})
+	require.NoError(t, jpeg.Encode(&jpegData, img, nil))
+
+	fakeServer := httptest.NewServer(http.HandlerFunc(func(writer http.ResponseWriter, _ *http.Request) {
+		writer.Header().Set("Content-Type", "image/jpeg")
+		_, _ = writer.Write(jpegData.Bytes())
+	}))
+	defer fakeServer.Close()
+
+	srv := NewMust(&server.Config{
+		URL:         fakeServer.URL + "/",
+		Timeout:     server.Duration{Duration: time.Second},
+		JPEGRetries: 1,
+	})
+	camera := &Camera{Number: 1, server: srv}
+
+	path := t.TempDir() + "/snap.jpg"
+	require.NoError(t, os.WriteFile(path, []byte("keep"), 0o600))
+	require.ErrorIs(t, camera.SaveJPEG(nil, path), ErrPathExists)
+
+	raw, err := os.ReadFile(path) //nolint:gosec // test temp path
+	require.NoError(t, err)
+	require.Equal(t, []byte("keep"), raw)
+}
+
+// Slow bodies used to fail with "missing SOI marker" / "context canceled" because
+// GetJPEG canceled the request context before reading the response body.
+func TestGetJPEGReadsBodyBeforeCancel(t *testing.T) {
+	t.Parallel()
+
+	var jpegData bytes.Buffer
+
+	img := image.NewRGBA(image.Rect(0, 0, 2, 2))
+	img.Set(0, 0, color.RGBA{R: 1, G: 2, B: 3, A: 255})
+	require.NoError(t, jpeg.Encode(&jpegData, img, &jpeg.Options{Quality: 90}))
+
+	payload := jpegData.Bytes()
+
+	fakeServer := httptest.NewServer(http.HandlerFunc(func(writer http.ResponseWriter, _ *http.Request) {
+		writer.Header().Set("Content-Type", "image/jpeg")
+		writer.WriteHeader(http.StatusOK)
+
+		flusher, _ := writer.(http.Flusher)
+		// trickle bytes so a premature context cancel would truncate the body
+		for i := 0; i < len(payload); i += 8 {
+			end := min(i+8, len(payload))
+
+			_, _ = writer.Write(payload[i:end])
+
+			if flusher != nil {
+				flusher.Flush()
+			}
+
+			time.Sleep(5 * time.Millisecond)
+		}
+	}))
+	defer fakeServer.Close()
+
+	srv := NewMust(&server.Config{
+		URL:         fakeServer.URL + "/",
+		Timeout:     server.Duration{Duration: 5 * time.Second},
+		JPEGRetries: 1,
+	})
+	camera := &Camera{Number: 1, server: srv}
+
+	got, err := camera.GetJPEG(nil)
+	require.NoError(t, err)
+	require.NotNil(t, got)
+
+	path := t.TempDir() + "/snap.jpg"
+	require.NoError(t, camera.SaveJPEG(nil, path))
+
+	raw, err := os.ReadFile(path) //nolint:gosec // test temp path
+	require.NoError(t, err)
+	require.GreaterOrEqual(t, len(raw), 2)
+	require.Equal(t, byte(0xFF), raw[0])
+	require.Equal(t, byte(0xD8), raw[1])
 }
 
 func TestMakeVideoURLUserinfoAndCodecs(t *testing.T) {

cameras_types.go

@@ -26,6 +26,17 @@ const DefaultEncoder = "/usr/local/bin/ffmpeg"
 // Those methods remux RTSP only; use StreamMJPG / StreamH264 for HTTP media.
 var ErrHTTPVideoUnsupported = errors.New("HTTP video remux unsupported; use RTSP (UseHTTP=false)")
 
+// ErrCameraUnavailable is returned when ++image responds HTTP 404 (camera offline / missing).
+var ErrCameraUnavailable = errors.New("camera unavailable (HTTP 404)")
+
+// ErrInvalidJPEG is returned when ++image body is not a JPEG (missing SOI marker).
+var ErrInvalidJPEG = errors.New("invalid JPEG format: missing SOI marker")
+
+const (
+	jpegFilePerm      = 0o600
+	jpegSOIPreviewMax = 64
+)
+
 // CameraArmMode locks arming to an integer of 0 or 1.
 type CameraArmMode rune