Add locked accessors for the fields Refresh() replaces - #94
Conversation
Refresh() swaps Cameras, Groups and Info under the server's mutex, but that mutex is unexported, so the doc telling callers to "use Rlock() on this struct" was impossible to follow: every reader raced the refresh. Apps hit this whenever Refresh() runs from a retry loop or event handler while requests read the camera list. GetCameras(), GetInfo() and GetGroups() read those fields under the read lock and return a snapshot. The library's own event stream and file listing now use them too, since both run outside the refreshing goroutine. Co-authored-by: Cursor <cursoragent@cursor.com>
Refresh() replaces Server.Cameras and Server.Info, and it runs from the retry loop, the event stream and the Telegram /refresh command while HTTP requests, chat wizards and the /debug/vars callbacks read those fields. The library had no way to read them safely; golift/securityspy#94 adds GetCameras(), GetInfo() and GetGroups(), so use them everywhere and pin the dependency to that commit. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Pull request overview
Adds thread-safe accessors for server state replaced by concurrent refreshes.
Changes:
- Adds locked camera, group, and server-info accessors.
- Migrates event and file readers to those accessors.
- Adds concurrent refresh coverage.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
securityspy.go |
Adds synchronized state accessors. |
securityspy_types.go |
Documents refresh synchronization requirements. |
refresh_concurrent_test.go |
Tests concurrent refreshes and reads. |
files.go |
Uses synchronized snapshots for file metadata. |
events.go |
Uses synchronized snapshots during event processing. |
Suppressed comments (2)
refresh_concurrent_test.go:42
- These
requireassertions may invoketesting.T.FailNowfrom the reader worker, which is not a supported use ofFailNow. Use a non-fatal report and explicitly return from the worker on failure.
require.NotNil(t, cams)
require.NotNil(t, cams.ByNum(3))
require.NotEmpty(t, cams.All())
refresh_concurrent_test.go:55
require.NotNilcan calltesting.T.FailNowfrom this worker goroutine, contrary to thetestingAPI contract. Report the failure non-fatally and return before dereferencinginfo.
require.NotNil(t, info)
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // when another goroutine may call Refresh(), which replaces it. | ||
| // The returned *Cameras is a snapshot: a later refresh builds a new one. | ||
| func (s *Server) GetCameras() *Cameras { | ||
| s.mu.RLock() |
There was a problem hiding this comment.
Good catch, fixed in a3ee38f — this would have been a real regression. RefreshContext now builds the info, camera list and groups into locals (camera wiring moved to wireCameras) and holds mu only for the three assignments; a new refreshMu keeps refreshes serialized with each other as before. Added TestRefreshDoesNotBlockReaders, which parks a refresh mid-request and asserts reads still come from the previous snapshot. It fails against the old locking exactly as you described: the reader blocks until the client timeout fires, --- FAIL ... (10.01s) ... context deadline exceeded.
| defer close(done) | ||
|
|
||
| for range 25 { | ||
| require.NoError(t, serverObj.Refresh()) |
There was a problem hiding this comment.
Correct, fixed in a3ee38f. FailNow may only be called from the goroutine running the test, so the workers now report with t.Errorf/t.Error and return; the loop conditions moved to an isDone helper. The require calls that remain are on the test goroutine, where they are fine.
RefreshContext held the write lock across the systemInfo round trip, so every accessor added in the previous commit would block for the length of that request — up to the client timeout when SecuritySpy is unreachable, which is exactly when an app's retry loop refreshes most often. Readers would have stalled behind a refresh that had nothing new to offer, while a perfectly good snapshot sat in memory. The request and camera wiring now build into locals and a separate mutex serializes refreshes, so the write lock covers only the three assignments. Co-authored-by: Cursor <cursoragent@cursor.com>
Summary
Refresh()replacesServer.Cameras,Server.GroupsandServer.InfounderServer.mu, but that mutex is unexported — so theServerdoc telling callers to "use Rlock() on this struct if there's a chance you may call methods while Refresh() is running" was impossible to follow. Every reader raced the refresh, and apps hit this as soon asRefresh()runs from a background retry loop or an event handler while requests read the camera list:GetCameras(),GetInfo()andGetGroups(), which read those fields under the read lock and return a snapshot (a later refresh builds a new one, so a retained pointer goes stale rather than mutating underneath you).RefreshContextno longer holds the write lock across the++systemInforound trip. It builds the replacement info, camera list and groups into locals (camera/schedule wiring moved towireCameras) and holdsmuonly for the three assignments; a newrefreshMukeeps refreshes serialized with each other as before. Without this, the accessors above would block for the length of the request — up to the client timeout when SecuritySpy is unreachable, which is exactly when a retry loop refreshes most often. Readers would stall behind a refresh that had nothing new to offer while a perfectly good snapshot sat in memory.custom()and event parsing, which run on the stream goroutine) and file listing (GetFile(),GetFiles()).ServerandRefresh()with what callers can actually act on.The fields stay exported, so this is backward compatible: existing single-goroutine callers keep working unchanged.
Test plan
go test -race ./...andgolangci-lint run ./...clean.TestRefreshConcurrentReadersruns a refresh loop against continuous readers for the whole refresh window. It passes with the accessors and reproduces the data race above when a reader is switched back toserverObj.Cameras.TestRefreshDoesNotBlockReadersparks a refresh mid-request and asserts reads are still served from the previous snapshot. Against the pre-fix locking it fails exactly as expected — the reader waits out the client timeout:--- FAIL (10.01s) ... context deadline exceeded.t.Errorfrather thanrequire, sinceFailNowmay only be called from the goroutine running the test.