From e7423b3cc3e0e4b40e290ba10741072976a1c2cc Mon Sep 17 00:00:00 2001 From: David Newhall II Date: Mon, 20 Jul 2026 13:02:09 -0700 Subject: [PATCH 1/2] Fix jpeg capture cancelling context --- cameras.go | 103 ++++++++++++++++++++++++--------------- cameras_internal_test.go | 98 +++++++++++++++++++++++++++++-------- cameras_types.go | 12 +++++ 3 files changed, 155 insertions(+), 58 deletions(-) diff --git a/cameras.go b/cameras.go index f14248d..622eb92 100644 --- a/cameras.go +++ b/cameras.go @@ -1,6 +1,7 @@ package securityspy import ( + "bytes" "context" "encoding/base64" "errors" @@ -175,66 +176,92 @@ 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) { - if ops == nil { - ops = &VidOps{} + data, err := c.fetchJPEGBytes(ops) + if err != nil { + return nil, err } - ops.FPS = -1 // not used for single image - - 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) - - continue - } - - jpgImage, err := jpeg.Decode(resp.Body) - _ = resp.Body.Close() - - if err != nil { - lastErr = fmt.Errorf("decoding jpeg: %w", err) - - continue - } - - return jpgImage, nil + jpgImage, err := jpeg.Decode(bytes.NewReader(data)) + if err != nil { + return nil, fmt.Errorf("decoding jpeg: %w", err) } - return nil, lastErr + return jpgImage, nil } // 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. +// 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 { if _, err := os.Stat(path); !os.IsNotExist(err) { return ErrPathExists } - jpgImage, err := c.GetJPEG(ops) + data, err := c.fetchJPEGBytes(ops) if err != nil { return fmt.Errorf("getting jpeg: %w", err) } - oFile, err := os.Create(path) //nolint:gosec // we are creating a file in a safe way. + if err := os.WriteFile(path, data, jpegFilePerm); err != nil { + return fmt.Errorf("writing jpeg: %w", err) + } + + return nil +} + +// fetchJPEGBytes downloads a still from ++image. The request context stays alive +// until the body is fully read — canceling earlier truncates the JPEG. +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 + + // Single short attempt — dead cameras must fail in ~2s, not stall /pics. + ctx, cancel := context.WithTimeout(context.Background(), c.jpegFetchTimeout()) + + resp, err := c.server.GetContextClient(ctx, "++image", c.makeRequestParams(ops), client) if err != nil { - return fmt.Errorf("os.Create: %w", err) + cancel() + + return nil, fmt.Errorf("getting image: %w", err) } - defer oFile.Close() - err = jpeg.Encode(oFile, jpgImage, nil) + data, err := io.ReadAll(resp.Body) + _ = resp.Body.Close() + + cancel() + if err != nil { - return fmt.Errorf("encoding jpeg: %w", err) + return nil, fmt.Errorf("reading image: %w", err) } - return nil + if resp.StatusCode == http.StatusNotFound { + return nil, fmt.Errorf("%w: %s", ErrCameraUnavailable, c.Name) + } + + if resp.StatusCode != http.StatusOK { + return nil, fmt.Errorf("%w: status %d (%d bytes)", server.ErrCmdNotOK, resp.StatusCode, len(data)) + } + + 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 data, nil +} + +// jpegFetchTimeout is intentionally short so offline/hung cameras fail fast in /pics. +func (c *Camera) jpegFetchTimeout() time.Duration { + return jpegFetchTimeoutSec * time.Second } // ToggleContinuous arms or disarms continuous capture via ++ssControlContinuous. diff --git a/cameras_internal_test.go b/cameras_internal_test.go index 259b34d..e00231a 100644 --- a/cameras_internal_test.go +++ b/cameras_internal_test.go @@ -7,6 +7,7 @@ import ( "image/jpeg" "net/http" "net/http/httptest" + "os" "testing" "time" @@ -15,39 +16,24 @@ import ( "golift.io/securityspy/v2/server" ) -func TestGetJPEGRetriesAndSucceeds(t *testing.T) { +func TestGetJPEGSucceeds(t *testing.T) { t.Parallel() - const retryAttempts = 4 - - const badAttempts = retryAttempts - 1 - - var ( - requests int - jpegData bytes.Buffer - ) + 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) { - requests++ - if requests <= badAttempts { - _, _ = writer.Write([]byte("not-a-jpeg")) - - return - } - 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: retryAttempts, + URL: fakeServer.URL + "/", + Timeout: server.Duration{Duration: time.Second}, }) camera := &Camera{Number: 2, server: srv} @@ -55,7 +41,79 @@ func TestGetJPEGRetriesAndSucceeds(t *testing.T) { got, err := camera.GetJPEG(nil) require.NoError(t, err) assert.NotNil(t, got) - assert.Equal(t, retryAttempts, requests) +} + +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}, + }) + camera := &Camera{Number: 2, Name: "X", server: srv} + + _, err := camera.GetJPEG(nil) + require.Error(t, err) + require.ErrorIs(t, err, ErrInvalidJPEG) +} + +// 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) { diff --git a/cameras_types.go b/cameras_types.go index b926c18..2b71929 100644 --- a/cameras_types.go +++ b/cameras_types.go @@ -26,6 +26,18 @@ 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 + jpegFetchTimeoutSec = 2 +) + // CameraArmMode locks arming to an integer of 0 or 1. type CameraArmMode rune From 9aea2c41cfb7564b3242d2eef07cf1670bffb651 Mon Sep 17 00:00:00 2001 From: David Newhall II Date: Mon, 20 Jul 2026 13:10:09 -0700 Subject: [PATCH 2/2] Address Copilot review on JPEG fetch. Restore configurable Timeout/JPEGRetries, use O_EXCL for SaveJPEG, and avoid reading full bodies on non-OK responses. Co-authored-by: Cursor --- cameras.go | 81 ++++++++++++++++++++++-------- cameras_internal_test.go | 104 ++++++++++++++++++++++++++++++++++++++- cameras_types.go | 5 +- 3 files changed, 164 insertions(+), 26 deletions(-) diff --git a/cameras.go b/cameras.go index 622eb92..b8c6f95 100644 --- a/cameras.go +++ b/cameras.go @@ -193,17 +193,33 @@ func (c *Camera) GetJPEG(ops *VidOps) (image.Image, error) { // 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 { - if _, err := os.Stat(path); !os.IsNotExist(err) { - return ErrPathExists - } - data, err := c.fetchJPEGBytes(ops) if err != nil { return fmt.Errorf("getting jpeg: %w", err) } - if err := os.WriteFile(path, data, jpegFilePerm); err != nil { - return fmt.Errorf("writing 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 @@ -211,6 +227,7 @@ func (c *Camera) SaveJPEG(ops *VidOps, path string) error { // 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{} @@ -220,8 +237,26 @@ func (c *Camera) fetchJPEGBytes(ops *VidOps) ([]byte, error) { client := c.streamHTTPClient() // Timeout=0; context bounds the whole fetch - // Single short attempt — dead cameras must fail in ~2s, not stall /pics. - ctx, cancel := context.WithTimeout(context.Background(), c.jpegFetchTimeout()) + var lastErr error + + for range c.server.JPEGTries() { + data, err := c.fetchJPEGBytesOnce(ops, client) + if err == nil { + return data, nil + } + + lastErr = err + // Offline / missing cameras won't recover within this call. + if errors.Is(err, ErrCameraUnavailable) { + return nil, err + } + } + + return nil, lastErr +} + +func (c *Camera) fetchJPEGBytesOnce(ops *VidOps, client *http.Client) ([]byte, error) { + ctx, cancel := context.WithTimeout(context.Background(), c.server.TimeoutDur()) resp, err := c.server.GetContextClient(ctx, "++image", c.makeRequestParams(ops), client) if err != nil { @@ -230,6 +265,23 @@ func (c *Camera) fetchJPEGBytes(ops *VidOps) ([]byte, error) { return nil, fmt.Errorf("getting image: %w", err) } + if resp.StatusCode == http.StatusNotFound { + _ = resp.Body.Close() + + cancel() + + return nil, fmt.Errorf("%w: %s", ErrCameraUnavailable, c.Name) + } + + 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) + } + data, err := io.ReadAll(resp.Body) _ = resp.Body.Close() @@ -239,14 +291,6 @@ func (c *Camera) fetchJPEGBytes(ops *VidOps) ([]byte, error) { return nil, fmt.Errorf("reading image: %w", err) } - if resp.StatusCode == http.StatusNotFound { - return nil, fmt.Errorf("%w: %s", ErrCameraUnavailable, c.Name) - } - - if resp.StatusCode != http.StatusOK { - return nil, fmt.Errorf("%w: status %d (%d bytes)", server.ErrCmdNotOK, resp.StatusCode, len(data)) - } - if len(data) < 2 || data[0] != 0xFF || data[1] != 0xD8 { preview := data if len(preview) > jpegSOIPreviewMax { @@ -259,11 +303,6 @@ func (c *Camera) fetchJPEGBytes(ops *VidOps) ([]byte, error) { return data, nil } -// jpegFetchTimeout is intentionally short so offline/hung cameras fail fast in /pics. -func (c *Camera) jpegFetchTimeout() time.Duration { - return jpegFetchTimeoutSec * time.Second -} - // ToggleContinuous arms or disarms continuous capture via ++ssControlContinuous. // // ToggleMotion and ToggleActions use similar ++ssControl* endpoints and work on diff --git a/cameras_internal_test.go b/cameras_internal_test.go index e00231a..d403548 100644 --- a/cameras_internal_test.go +++ b/cameras_internal_test.go @@ -8,6 +8,7 @@ import ( "net/http" "net/http/httptest" "os" + "sync/atomic" "testing" "time" @@ -43,6 +44,49 @@ func TestGetJPEGSucceeds(t *testing.T) { assert.NotNil(t, got) } +func TestGetJPEGRetriesAndSucceeds(t *testing.T) { + t.Parallel() + + const retryAttempts = 4 + + const badAttempts = retryAttempts - 1 + + var ( + requests atomic.Int32 + 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) { + n := requests.Add(1) + if n <= badAttempts { + _, _ = writer.Write([]byte("not-a-jpeg")) + + return + } + + 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: retryAttempts, + }) + + camera := &Camera{Number: 2, server: srv} + + got, err := camera.GetJPEG(nil) + require.NoError(t, err) + assert.NotNil(t, got) + assert.Equal(t, int32(retryAttempts), requests.Load()) +} + func TestGetJPEGRejectsNonJPEG(t *testing.T) { t.Parallel() @@ -52,8 +96,9 @@ func TestGetJPEGRejectsNonJPEG(t *testing.T) { defer fakeServer.Close() srv := NewMust(&server.Config{ - URL: fakeServer.URL + "/", - Timeout: server.Duration{Duration: time.Second}, + URL: fakeServer.URL + "/", + Timeout: server.Duration{Duration: time.Second}, + JPEGRetries: 1, }) camera := &Camera{Number: 2, Name: "X", server: srv} @@ -62,6 +107,61 @@ func TestGetJPEGRejectsNonJPEG(t *testing.T) { 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) { diff --git a/cameras_types.go b/cameras_types.go index 2b71929..7db13b2 100644 --- a/cameras_types.go +++ b/cameras_types.go @@ -33,9 +33,8 @@ var ErrCameraUnavailable = errors.New("camera unavailable (HTTP 404)") var ErrInvalidJPEG = errors.New("invalid JPEG format: missing SOI marker") const ( - jpegFilePerm = 0o600 - jpegSOIPreviewMax = 64 - jpegFetchTimeoutSec = 2 + jpegFilePerm = 0o600 + jpegSOIPreviewMax = 64 ) // CameraArmMode locks arming to an integer of 0 or 1.