Three defects in the dispatch path.
Deleted was still a trigger. The Initialized guard was a denylist of one
value, so a property removal with an active-looking payload passed
straight through. Both Initialized and Deleted are announcements about a
property, not motion starting, so accept the transitions instead:
Changed, and Unknown for the events that omit the optional attribute.
ev.Topic reached the log unmodified. It is camera-controlled, unbounded
and unfiltered, and logrus's coloured text formatter — the default —
writes the message without quoting, so an embedded newline forges whole
log entries. A compromised camera could fabricate ERROR lines or spoof
another device's id in the logs an operator is reading to diagnose that
camera. Escape control characters and bound the length; the reject path
logs every event received, so an oversized topic was also a cheap way to
evict a container's retained history.
The trigger line was logged before the send, so an event dropped on a
full channel or at shutdown left a line claiming a recording that never
started. Log it in the send case.
A camera replays the current state of every property topic as
PropertyOperation=Initialized whenever a pull-point subscription is
created. dispatchEvent looked only at Kind and State, so any motion
property that happened to be active at that moment counted as a fresh
trigger — meaning every reconnect restarts a recording, and a flapping
subscription manufactures motion with no motion.
Observed on a camera whose pull-point was being recreated every ~18s:
each recreate replayed ~90 property events, and once real motion made
the VMD property active the replays kept re-triggering it.
Rejects Initialized specifically rather than accepting only Changed.
PropertyOperation is optional per WS-Notification and absent on many
non-property events, which decode reports as PropertyUnknown; those are
real events and must still trigger.
dispatchEvent logged only rejected events, so the topic that actually
started a recording was invisible — the only way to identify it was to
enumerate every rejected topic and reason about what was left. On a
camera emitting 18 distinct topics that is not a diagnosis.
Log the Kind and topic on the dispatch path too, at debug, matching the
reject line's shape so both sides of the decision grep the same way.
Audit against CLAUDE.md's 'default to no comments; only when WHY is
non-obvious'. Net ~30 lines removed.
Dropped (rot-prone or redundant)
--------------------------------
* 'matching what the pixel-diff detector emits' — references a
sibling file's behaviour.
* 'Timestamp in seconds matches what computervision/main.go emits;
downstream consumers (capture/main.go) tolerate...' — both
cross-file references; classic 'will rot when the sibling
changes'.
* 'Motion-stop wiring into the recorder state machine is tracked as
a follow-up; today the recorder uses a fixed PostRecording
timeout' — PR-description content masquerading as a code comment.
* 'happens only on ctx cancel today (library handles its own
reconnect)' — 'today' is a red flag; either drop or assert via
test, not narrate.
* dispatchEvent's first paragraph restating what the function does.
* runStreamOnce's first sentence (WHAT).
* isONVIFMotionEnabled's reference to sibling Capture fields
('unlike Recording / Motion / Snapshots which default to
enabled').
Kept (real WHYs)
----------------
* The shutdown-race rationale on dispatchEvent's ctx guards.
* logStreamError's severity-mapping rationale.
* The library-handles-reconnect-but-not-initial-connect rationale
for the backoff constants.
* The recovering-flag rationale (on-call ops use case).
* The flag-read-once invariant on HandleONVIFEventStream.
Addresses the critical and important findings from the second review of
the agent integration. TDD followed locally: tests were written first
and confirmed RED against the previous implementation before the fix
turned them GREEN.
Critical fixes
--------------
* Shutdown-race panic (concurrency P0): the 3s gap between the agent's
ctx cancel and close(HandleMotion) was reachable by a buffered event
delivered after cancel, where dispatchEvent's send-with-default
select would panic on the closed channel. dispatchEvent now takes
ctx, has a pre-check after the kind/state/recording filters, and
the send select includes a <-ctx.Done() arm. Pinned by
TestDispatchEvent_CtxCancelledAndHandleMotionClosed_DoesNotPanic
(asserts NotPanics; current code without the fix panics).
* No retry on initial connect (Go P0 + ops P1): previously the
goroutine exited permanently if ConnectToOnvifDevice or
stream.NewStream failed at agent start — a brief boot-time DNS or
network blip silently disabled ONVIF until restart. Construction is
now wrapped in a retry loop with exponential backoff (1s -> 5min),
matching what cloud.HandleHeartBeat does for its ONVIF connection
attempts. The library handles in-stream recovery already; this
covers the gap the library cannot see.
* Strict 'true' match (Go P0): isONVIFMotionEnabled now normalises
case and trims whitespace, so 'True', 'TRUE', ' true ' all enable
the feature. Pinned by TestIsONVIFMotionEnabled_CaseAndWhitespace.
Important fixes
---------------
* Empty DeviceID fallback (Go P1): resolveDeviceID falls back from
configuration.Name to camera.ONVIFXAddr to 'unknown' so log lines
and metrics always have a useful identifier. Pinned by
TestResolveDeviceID_FallbackChain.
* Recovery log (ops P1): the run loop tracks a 'recovering' flag set
when an ErrPullFailed/ErrRecreateFailed lands on Errors and cleared
on the first successful Event. Logs an Info 'event stream recovered'
line so on-call operators can see error streaks clear, instead of
waking up to ERROR with no closure.
* Misconfig log bumped Info -> Warning so the
'ONVIFXAddr is empty' line stands out from the heartbeat noise.
Tests
-----
events_test.go covers the dispatch contract end-to-end:
* Motion+Active -> HandleMotion (happy path).
* Motion+Inactive ignored (motion-stop is a documented follow-up).
* Non-motion kinds ignored.
* Recording='false' gates the send.
* Full HandleMotion drops rather than blocks.
* Ctx-cancelled + closed HandleMotion does not panic.
* isONVIFMotionEnabled handles case and whitespace.
* resolveDeviceID fallback chain.
go.mod / go.sum: testify moved from indirect to direct dependency.
Deferred (out of scope for this commit, tracked as follow-ups):
* Heartbeat surface for ONVIF state ('disabled|running|failed') —
requires a Cloud.go change beyond this integration's scope.
* OTel span/metric for stream lifecycle.
* Runtime toggle without restart (config-reload).
* Replace-directive layout documentation — separate docs commit.
Resolves kerberos-io/agent#173. When Capture.ONVIFMotion='true' the
agent opens a stream.NewStream against the configured ONVIF endpoint
and forwards Motion+Active events to HandleMotion, so AXIS cameras
(and any other ONVIF-conformant device) can drive motion-triggered
recording without relying on the pixel-diff detector.
Why this shape
--------------
Maintainer @cedricve's direction on #194 was 'extend the ONVIF
library first to expose a go channel and hide the protocol complexity'.
That work landed in kerberos-io/onvif (event/stream sub-package);
the agent integration is now a thin consumer: connect to the device,
wrap it in NewStream, range over Events, push Motion events at the
existing HandleMotion channel.
Design choices
--------------
* New file machinery/src/onvif/events.go keeps the new code separate
from the existing PTZ/IO-focused onvif/main.go so reviewers can
read it without scrolling.
* Opt-in via Capture.ONVIFMotion. Defaults preserve the current
pixel-diff behaviour so nothing changes for existing users.
* DispatchEvent only fires on StateActive — the leading edge. Motion
STOP requires the recorder state machine to accept an explicit
stop signal which today does not exist; tracked as a follow-up so
this first PR stays small.
* Non-blocking send to HandleMotion: drop ONVIF motion events when
the channel is full rather than block the stream goroutine and
starve subscription renewal.
* Error logging routes by typed-error category from the library:
ErrRecreateFailed is loud (camera may be offline), ErrPullFailed
and ErrRenewFailed are debug-level (auto-recovers).
* Goroutine lifetime tied to *communication.Context, the same
cancellable context the agent uses to restart on config change.
Not in this commit (intentionally deferred to follow-ups)
---------------------------------------------------------
* Motion STOP wiring into capture/main.go's recorder state machine.
* Replacing the ad-hoc CreatePullPointSubscription / GetEventMessages
polling in cloud/Cloud.go with a stream consumer for DigitalInput
and DigitalOutput events. The current code keeps working; the
stream is purely additive.
* Removing the temporary 'replace' directive on
github.com/kerberos-io/onvif once the event/stream changes are
tagged upstream.
Adds Capture.ONVIFMotion and the matching AGENT_CAPTURE_ONVIF_MOTION
environment override. When set to 'true', the agent will open an
ONVIF event stream against the configured camera and route Motion
events into the existing HandleMotion channel (wired up in the
next commit).
Defaults to empty (disabled), so existing deployments using the
pixel-diff motion detector see no behaviour change.
Expand discovery and classification to better identify device vendors and stream paths by adding new brand aliases/profiles (including D-Link, Trendnet, Lorex, Honeywell, Pelco, and TOA), improved realm matching, and hostname-based brand hints. Add HTTP body fingerprinting for OEM/rebadged devices, introduce audio-device detection with a new `is_audio` API field, and prevent camera RTSP guessing/fallback URLs for audio-only devices while updating discovery logging labels.
Adds Linksys camera detection across ONVIF heuristics by introducing a dedicated brand profile, realm aliases, and banner fingerprinting. It also prioritizes Linksys-specific RTSP endpoints (including /ONVIF/channel1 and /ONVIF/channel2) and includes fallback stream paths used by Cisco/Linksys models.
Extend device discovery to generate RTSP stream candidates using built-in brand profiles, RTSP DESCRIBE probing, auth-realm parsing, and port hints. Add `RTSPStreams`/`RTSPStream` to API responses, prefer verified stream URLs as primary `RTSPURL`, and let stronger RTSP-derived brand/model signals refine detected camera metadata. Also add focused unit tests for discriminating vs non-discriminating devices, realm-based brand/model detection, and generic fallback behavior.
Introduces a new ONVIF/network discovery pipeline that combines WS-Discovery, subnet-aware host/port scanning, banner fingerprinting, and MAC vendor enrichment to identify likely cameras. Adds API and CLI support for discovery options (`/api/camera/discover`, `-subnet`), plus a richer discovered-device response model. Also adds MQTT `verify-stream` handling to probe RTSP streams and return codec/resolution/fps, and persists detected stream FPS into config for main/sub streams.
Removes sensitive credentials, private keys, and personal configuration values from config.json. Clears RTSP URLs, hub keys, encryption/signing keys, kstorage credentials, and resets various settings to neutral defaults.
Changes PixelChangeThreshold from int to *int so nil (unset) defaults to 150, while 0 explicitly disables motion detection. Updates ProcessMotion to handle the new three-state logic and also emits pixelChangeThreshold in MQTT motion messages for live view visualization.
Replace unsafe fmt.Sprintf JSON formatting with proper struct marshalling in cloud.go. Add rawJSONOrEmptyArray() helper to safely handle json.RawMessage with fallback to empty arrays. Enhance motion detection overlay data in computervision by adding main stream dimensions (mainWidth/mainHeight) alongside motion frame dimensions, enabling accurate live-view scaling of detected regions.
Refactor motion detection logic to enable motion region visualization during continuous recording. Motion detection now runs in continuous mode when a motion region is configured, allowing live-view overlay display without triggering motion-based recording. In continuous mode without regions, motion detection is skipped as before. Updated conditional logic and added clarifying comments explaining the different code paths.
Report whether a camera is in continuous recording mode (24/7) to the Hub. This allows the Hub live view to disable the manual record button, which is a no-op when the camera is already recording continuously.
Add heartbeat-aware lifecycle management for manual/live-view recordings. The agent now tracks manual recording start time and viewer heartbeats, auto-stops recordings when heartbeats lapse or a max duration is reached, and clears state on stop/restart. MQTT recording payloads gain a `heartbeat` flag so keep-alives refresh active sessions without unintentionally restarting recordings after auto-stop.
Corrects the default AGENT_TURN_URI value in the configuration table from port 348 to 3478. This fixes a typo and aligns the TURN URI with the standard/STUN port used elsewhere in the README.
Add livePreviewHttp flag to the device payload and log whether HTTP preview transport is enabled or disabled. Track the transport actually used (HTTP vs MQTT) with a lastTransport variable to avoid per-frame log spam and emit informative logs only when the transport changes, including fallback reasons (Hub not configured or HTTP upload failure). Capture HTTP publish errors to include in fallback messages, and lower the per-frame MQTT publish log level from Info to Debug. Small comment added explaining the logging behavior.
