Both VMD rules shipped with positive cases only. The file already keeps
a "Substring guards" section because Classify matches with
strings.Contains, so a needle that is a prefix of a sibling name
silently captures it — the same reason MyRuleDetector's sub-rules are
whitelisted individually.
Pin the two properties the needles rely on: the trailing slash makes
them match a whole path segment, and VMD3 is scoped to RuleEngine.
Verified by dropping each from the rule and watching these fail.
Also drops a site-specific note from the VMD 3 comment — where it was
first seen is not something an upstream reader can act on.
Two gaps in the previous commit's guard.
Strict inequality was not enough. A client timeout one millisecond
above PullTimeout passed, and the test pinned that as valid — but the
client ceiling also has to cover dial, TLS and the response transfer on
top of the poll it outlasts, which on a cellular bearer is hundreds of
milliseconds. Require minClientHeadroom (5s) above PullTimeout.
The error was a bare fmt.Errorf, so callers could not tell it from the
transient pull/renew/recreate failures they retry. A consumer that
retries this one loops forever on a configuration that can never
succeed. ErrInvalidOptions is a sentinel they can short-circuit on.
Zero stays accepted: it is the SDK's default when a caller passes no
client, so rejecting it would break every default consumer. The comment
no longer claims that is safe — the caller interface documents that ctx
cannot interrupt an in-flight SOAP call, so an unbounded client is the
one case nothing can unwedge.
VMD 3 is the firmware-builtin motion rule, published as
tns1:RuleEngine/tnsaxis:VMD3/vmd3_video_<N>. It predates the VMD 4 ACAP
and lives under RuleEngine rather than CameraApplicationPlatform, so the
VMD 4 rule added in the previous commit does not cover it.
Confirmed against a deployed AXIS camera: with debug logging on, its
event stream emits vmd3_video_1 on every motion trigger, and every one
was discarded as KindUnknown.
The VMD 4 rule stays — both generations are in the field.
PullMessages is a long-poll: the camera holds the connection open for
up to PullTimeout waiting for an event. http.Client.Timeout bounds the
whole exchange — dial, write, wait-for-headers — and starts before the
camera has parsed the request, so a client ceiling equal to or below
PullTimeout expires first on every interval with no event.
The failure mode is quiet and easy to misread. Pulls fail continuously,
but the stream stays alive because ReconnectAfterFailures recreates the
subscription, and each recreate makes the camera replay its full
property state. Events keep arriving, in bursts, on the reconnect
cadence rather than when they happen — so it reads as a slow camera
rather than a misconfiguration.
Observed in the field with both values at 5s: every pull timed out,
recovery landed after exactly 3 failures, and ~90 property-state events
were replayed every 18s.
Validated in NewStream, before the subscription call, since the config
can only fail. A zero client timeout stays legal — unbounded is safe
because the pull loop is already bounded by ctx.
The topic table covered the AXIS Guard suite (MotionGuard, FenceGuard,
LoiteringGuard) but not VMD — the stock motion app shipped on every
AXIS camera, and the one an installer configures before reaching for a
paid Guard product.
VMD publishes on tnsaxis:CameraApplicationPlatform/VMD/Camera<N>Profile
<ID>, which fell through every rule to KindUnknown. Consumers that key
recording off KindMotion therefore never recorded, and because an
unrecognised Kind is normally discarded at debug level the failure
presents as silence rather than an error.
Found on a site where an AXIS camera with VMD and a motion recording
trigger produced no recordings across a 9h41m window while its ONVIF
subscription stayed healthy throughout.
The reference-parameter-to-SOAP-header mapping rule (with the
wsa:IsReferenceParameter='true' attribute) lives in WS-Addressing 1.0
SOAP Binding §3.4 (Binding Message Addressing Properties), not Core
§3.1 (Abstract Property Definitions). The earlier citations were
wrong on both the section number and the document. Corrected across
doc comments, test descriptions, and the PR description.
The "ReferenceParameters can appear in any endpoint reference" claim
in the anchored-extraction test now cites Core §2.1 (Information
Model for Endpoint References), which is where the [reference
parameters] property is defined on the abstract EPR.
Verified against the W3C Recommendations:
- https://www.w3.org/TR/ws-addr-core/ §2.1
- https://www.w3.org/TR/ws-addr-soap/ §3.4
Also adds PR_event_stream_axis_compat.md — the branch's PR
description, framed independently of the prior event/stream PR.
Critical
- Two-step lock race in renewLoop: getPullPoint() + pullPointGen()
were separate Lock/Unlock pairs, leaving a window in which a
concurrent setPullPoint could advance gen between the two reads.
The renew then ran against ref-N but believed its captured gen
was N+1, and updateGrantedTerminationIfGen "succeeded" writing
the old subscription's grant onto the new one — same class of
bug the generation counter was meant to fix. New snapshotPullPoint
accessor reads both under one lock.
- wssePasswordRE was missing the close-tag-or-EOF alternative that
wsseSecurityRE got last round; a <Password> element truncated at
the 64 KiB error cap escaped redaction. Pattern is now symmetric.
Important
- Dropped unused (granted, now) params from nextRenewIntervalAfterError;
only opts.RetryBackoff is consulted. Tests adjusted.
- Moved gen field next to pullPointMu with explicit guard comment.
- Inlined the single-use rootTag helper; nil case handled directly.
- Loosened TestNextRenewIntervalAfterError_FallsBackToRetryBackoff
so future jitter doesn't break it.
Docs
- Extended trust-boundary godoc on SendSoap* to name the full set of
weaponisable WS-* headers (wsa:To/ReplyTo/FaultTo/MessageID,
wsu:Timestamp).
- Mirrored the trust-boundary warning on gosoap.AddStringHeaderContents
so library consumers see it at the package entry point too.
- Noted that updateGrantedTerminationIfGen intentionally doesn't bump
gen (would defeat rotation detection).
- Documented the wsseSecurityRE truncation-branch trade-off
(max-redact > max-context for log lines).
Tests
- TestSnapshotPullPoint_AtomicReadOfRefAndGen pins the new accessor.
- TestEnrichSOAPErr_RedactsTruncatedPasswordOutsideSecurity exercises
the now-symmetric password redaction.
- TestEnrichSOAPErr_RedactsMultipleSecurityBlocks pins existing
multi-block behaviour.
- TestBuildRefParamsHeader_RejectsNonReferenceParametersRoot_Table
covers vendor-suffix, multi-root, and unrelated-element cases.
- One-line comment on the &Stream{} tests explaining nil-now safety.
Critical
- Renew busy-loop on persistent failure: after a failed
renewPullPoint, the loop re-read the (now in-past)
GrantedTermination and nextRenewInterval floored to 1s, hammering
the camera at 1 Hz until reconnect. nextRenewIntervalAfterError
decouples the failure path from the stale grant and backs off
at opts.RetryBackoff. renewLoop also routes through s.now() so
test clocks can drive it deterministically.
- Lost-update race on GrantedTermination: a renew result for an
old subscription could overwrite the grant on a new one if
attemptRecreate swapped pullPoint mid-flight. A generation
counter on Stream tracks subscription rotation; the renew loop
captures the generation before the SOAP call and discards the
result if the subscription was rotated.
- Credential leak when <Security> straddled the 64 KiB error cap:
the non-greedy regex required a closing tag and missed the
truncated case. wsseSecurityRE now matches close-tag-or-EOF.
Belt-and-braces wssePasswordRE redacts <Password> elements
outside any Security wrapper.
- addHeaderChildren accepted well-formed-but-element-free input
and produced a header-less request. Now errors out.
Important
- Wrapper detection in buildRefParamsHeader was HasSuffix-based and
misfired on children named *ReferenceParameters. Replaced with the
unambiguous wrapper-only contract: input must be the full
<*:ReferenceParameters> element returned by extractReferenceParameters.
- Renamed SoapOption → SendSoapOption and WithHeader → WithSOAPHeader
to disambiguate at call sites.
- Renamed WithHeader's `xml` parameter to headerContent to avoid
shadowing the encoding/xml package name.
- Documented terminationTimeRE's "first match only" semantics so
nobody re-uses it from PullMessages context where multiple
TerminationTime elements appear.
- goleak is now a direct require (go mod tidy).
Performance
- Added gosoap.AddStringHeaderContents (plural) for multi-root
header content. AddStringHeaderContent remains as-is for
backwards compatibility with external consumers. Device.go's
addHeaderChildren workaround is gone — one etree parse per
SendSoapWithOptions call instead of two.
Migration
- Device.go's own CallOnvifFunction and the three examples now
call SendSoapWithOptions, modelling the canonical path.
Docs / tests
- Trust-boundary warning on SendSoapWithHeader/Options godoc.
- Comments on createPullPointResp/Alt explain why TerminationTime
is intentionally omitted (renew timing fixtures).
- New tests: digest retry strips WS-Security, duplicate
WithSOAPHeader is last-wins, malformed TerminationTime yields
zero, margin==base falls to base/2, empty SubscriptionReference
yields empty ref params, Security straddling cap is redacted,
bare Password redacted, wrapper-only contract is enforced.
Critical
- Device.SendSoapWithHeader now parses the supplied header content
with etree and adds each top-level child as its own SOAP Header
block. gosoap.AddStringHeaderContent only accepts a single root
element; previously a multi-child ref-params header silently
produced a header-less request because the parse error was
discarded. Errors are now propagated.
- enrichSOAPErr scrubs <*:Security> blocks from response bodies
before fault extraction or excerpt slicing so a camera that
echoes the WS-Security header in a fault response cannot leak
Username/Password into operator logs.
Important
- extractSOAPFault falls back to the SOAP 1.2 Subcode (e.g.
ter:InvalidArgs) when Reason/Text is empty — consistent with
enrichSOAPErr and surfaces actionable detail on 200-OK fault
bodies reached via unmarshalNode.
- subscriptionRef captures the camera-granted TerminationTime
from CreatePullPointSubscription and Renew responses. renewLoop
schedules from it via the new nextRenewInterval helper so we
never miss a renew when the camera grants less than requested.
renew is now a sleep-loop driven by the latest granted time.
- enrichSOAPErr reads at most 64 KiB from the body (vs. 10 MiB
on success paths). Fault bodies are always small; the prior cap
let a wedged camera churn 10 MiB/s through the retry loop.
Suggestions
- extractReferenceParameters anchors to <SubscriptionReference> so
a wsa:ReplyTo / wsa:FaultTo that also carries ReferenceParameters
elsewhere in the envelope cannot leak through and break PullMessages.
- buildRefParamsHeader accepts either raw children or the full
<*:ReferenceParameters> wrapper, and propagates ancestor xmlns:*
onto each child so a vendor that declares the prefix on the
parent (not the child itself, as AXIS does) still produces valid
standalone children on the wire.
- SendSoapWithHeader documents that xmlHeaderContent must be
well-formed XML and that the caller is responsible for escaping
any externally sourced data.
- Error-message ordering is now context-first
("SOAP fault: X: <wrapped err>") per Go convention.
- Dead headerEnd slicing removed from the SendSoapWithHeader test.
Tests
- End-to-end multi-child wiring through pullMessages.
- Digest auth retry preserves the injected header.
- Malformed-XML header content fast-fails before any request.
- buildRefParamsHeader malformed / whitespace-only edge cases.
- goleak.VerifyTestMain in event/stream catches any pull/renew
goroutine that outlives its Stream.
No new behavioural surface added to onvif core; SendSoap retains
its signature, SendSoapWithHeader is the only new public method.
AXIS encodes pull-point subscription identity in
<wsa:ReferenceParameters> inside the CreatePullPointSubscription
response — a generic /onvif/services endpoint plus a
<dom0:SubscriptionId> child — rather than a per-subscription URL.
We were discarding the ReferenceParameters and POSTing to the
generic endpoint, which AXIS rejected with ter:InvalidArgs on every
PullMessages/Renew/Unsubscribe.
Per WS-Addressing 1.0 §3.1 each reference parameter MUST be echoed
as a SOAP Header block carrying wsa:IsReferenceParameter="true".
- subscriptionRef now carries Address + the verbatim
ReferenceParameters inner XML extracted from the create response.
- buildRefParamsHeader walks the children, adds the attribute, and
produces the SOAP Header content.
- pullMessages, renewPullPoint, unsubscribePullPoint switch from
SendSoap to a new SendSoapWithHeader path on the caller interface.
- onvif.Device gains SendSoapWithHeader as a thin variant of
SendSoap (existing SendSoap is now a one-liner delegating to it
with empty header content, so all external callers are unaffected).
Verified end-to-end against an AXIS camera at 192.168.1.10: pulls
now stream the full topic tree (VMD, Object Analytics, IO, storage,
hardware-failure topics) instead of looping on ter:InvalidArgs.
Pull/renew/recreate/unsubscribe errors against a misbehaving camera
previously surfaced as "Post with digest error: 400: 400 Bad Request"
— operators had no way to tell auth failure from a malformed request
from an expired subscription. The body always carried the answer; we
just discarded it.
enrichSOAPErr now reads the response body alongside transport errors
and appends, in order of preference:
1. SOAP 1.1 faultstring / SOAP 1.2 Reason/Text — the reason text.
2. SOAP 1.2 Subcode/Value — AXIS routinely sends an empty Text and
leaves Subcode as the only actionable signal (ter:InvalidArgs etc).
3. Raw body excerpt, truncated to maxErrExcerpt — for non-SOAP
bodies (HTML error pages, plain text) without flooding logs.
The original error is preserved via %w so errors.Is/As in
logStreamError keep working. Wired into all four SOAP call sites
(createPullPoint, pullMessages, renewPullPoint, unsubscribePullPoint).
Concurrency audit (third review) flagged that caller.SendSoap is not
ctx-aware: cancelling ctx does not unblock a pull or renew goroutine
parked in the underlying http.Client.Do. The previous Close()
unconditionally did <-s.done before its 5s unsubscribe timeout, so a
wedged SendSoap could hang Close indefinitely — taking the agent's
shutdown down with it.
Adds closeDrainTimeout (5s) to bound the wait for the run goroutines
to exit. When the drain times out:
* Close returns a 'did not drain' error so the caller can move on.
* Unsubscribe is skipped; the subscription expires at the camera
once InitialTermination elapses without a Renew.
* The wedged goroutines exit later, when the HTTP transport
eventually gives up. They are effectively leaked until then —
documented in the caller interface comment as the contract
callers must accept (or fix, by configuring an http.Client.Timeout).
The caller interface doc-comment now states both invariants
explicitly: must be goroutine-safe AND must enforce its own per-
request timeout, because we cannot from here.
Test
----
TestClose_BoundedWhenLoopsStuckOnHungHTTP: drives the fakeCaller
with blockAllSendSoap (new flag) so every SendSoap parks. Waits for
pullLoop to actually reach the blocked SendSoap before calling
Close (a race the previous attempt had: Close raced the loop and
exited via the ctx pre-check). Asserts Close returns within
closeDrainTimeout + 2s slack with a drain-timeout error.
Other concurrency audit findings disposition
--------------------------------------------
* unsubscribe goroutine leaks past 5s: intentional, already
documented at closeUnsubscribeTimeout.
* now func() time.Time data race: written once before goroutines
start; safe by happens-before. Tests do not swap it today.
* closeOnce self-deadlock if Close called from inside a loop:
no path exists; not exposed via the API.
Audit against the standard 'default to no comments; only add one when
the WHY is non-obvious'. Net: 238 lines removed across 8 files, no
behaviour change, tests still pass -race.
What went
---------
* Section banners (// ---------- Motion ----------): noise once
per-rule citations exist.
* Per-rule 'Data: IsMotion (xsd:boolean)' wire-format lines in
topics.go: that's WHAT; the spec citation carries WHY.
* Per-field doc on Event struct restating each field name (// Kind
is the normalized event category) and the type-doc preamble.
* Stringer doc comments ('// String implements fmt.Stringer.') and
similar conventional-method noise.
* 'Used by ErrPullFailed / ErrRenewFailed / ErrRecreateFailed' in
the Op doc — the rule-named anti-pattern.
* doc.go Invariants and Reconnect sections duplicating per-function
docs.
* Internal helper doc-comments restating what the function does
(surfaceError, run, simpleItemsToMap first sentence, etc.).
What stayed
-----------
* Every spec / vendor-doc citation in topics.go.
* Race-condition WHY in stream.go run() close ordering.
* Workaround WHY in renew.go (absolute datetime vs duration).
* WS-BaseNotification UTC rationale + vendor format list in
decode.go.
* Fleet-sizing and thundering-herd rationale in reconnect.go.
* Stream consumer invariants (NewStream synchronous I/O, Errors
non-blocking, Close idempotent + bounded).
The change matches the codebase's stated style (CLAUDE.md): WHY only,
no WHAT, no cross-file references, no current-task narration.
Previously stream.go was a 621-line monolith holding the Stream type,
SOAP plumbing, pull loop, renew loop, recreate logic and jitter. The
test side had grown five orphan files (renew_test.go, reconnect_test.go,
soap_test.go, jitter_test.go, coverage_test.go) with no matching source
files. The mismatch made it harder than necessary to find the code
that backed a given test.
This commit splits stream.go by concern so each source file has its
own test file alongside it. Files <100 LOC (errors, jitter) were folded
into their conceptual parents rather than left as fragments.
New layout — 8 source + 8 test + helpers (test utility) + doc:
stream.go <-> stream_test.go Stream type, Options, lifecycle
soap.go <-> soap_test.go SOAP plumbing + fault detection
renew.go <-> renew_test.go Renew loop and absolute time
reconnect.go <-> reconnect_test.go Pull loop, recreate, jitter
decode.go <-> decode_test.go NotificationMessage -> Event
types.go <-> types_test.go Event types + typed errors
topics.go <-> topics_test.go Classifier table
doc.go Package godoc landing page
helpers_test.go waitFor (test-only utility)
Mergers
-------
* errors.go (typed error wrappers, 42 LOC) -> types.go. ErrPullFailed /
ErrRenewFailed / ErrRecreateFailed are part of the type system, not a
separate concern.
* jitter.go (40 LOC) -> reconnect.go. jitter is an implementation detail
of attemptRecreate, used nowhere else.
Test distribution
-----------------
* coverage_test.go was a catch-all; tests moved to the file matching
the function under test:
- Close*, NewStream_*, FakeCaller_* -> stream_test.go
- DisableReconnect_*, RecreateResets_*, PullPointMutation_* ->
reconnect_test.go
- Decode_*, ExtractState_* -> decode_test.go
* soap_test.go shed the two orphans that did not belong there:
- TestRenew_SendsAbsoluteDateTimeNotDuration -> renew_test.go
- TestClose_BoundedByTimeoutOnHungUnsubscribe -> stream_test.go
* errors_test.go's pure type tests -> types_test.go
* errors_test.go's Stream-integration tests -> reconnect_test.go
* jitter_test.go -> reconnect_test.go
No behaviour change. Test suite passes -race clean.
Adds the missing test coverage flagged by the test-rigor reviewer.
Coverage / behaviour
--------------------
* TestClose_ReturnsUnsubscribeError: previously closeErr plumbing was
effectively dead code in the suite. Inject an Unsubscribe failure
and assert the error wraps it.
* TestNewStream_CtxAlreadyCancelled: pins the behaviour for a
pre-cancelled parent context (construction succeeds because
createPullPoint does not consult ctx; run goroutine exits
immediately and Events closes).
* TestStream_DisableReconnectKeepsRetryingOriginalEndpoint: proves
the new opt-out actually disables CreatePullPoint recreate.
* TestStream_RecreateResetsFailuresAndBackoffOnSuccess: locks the
attemptRecreate success path resetting *failures and *backoff so a
later failure does not accidentally enter exponential backoff
immediately.
Race detection
--------------
* TestStream_PullPointMutationVisibleToRenewLoopUnderRace: drives the
pullPoint write-by-pullLoop / read-by-renewLoop race so -race
actually exercises the mutex critical sections. Previously the
mutex was structurally correct but no test produced contention.
Decoder edge cases
------------------
* TestDecode_PropertyOperationIsCaseSensitive: per WS-Notification
§3.3, values are PascalCase. Lowercased forms fall through to
PropertyUnknown.
* TestDecode_StateValueTrimsWhitespace: explicit assertions for
' true ', tabs, newlines and whitespace-only.
* TestDecode_SimpleItemEmptyValueIsUnknownState: empty value yields
StateUnknown but the empty entry is still preserved in Data map.
* TestDecode_DeviceTimeAdditionalLayouts: the +0200 compact offset
and naked-no-TZ formats added in the hardening commit.
* TestDecode_DeviceTimeStillRejectsNonsense: the broader layout list
did not start accepting garbage.
* TestExtractState_FirstBooleanLikeWins: uses explicit slice
construction (pair{k,v} -> SimpleItem) so the assertion does not
depend on map iteration order, the latent flake risk in the AOA
test pointed out by the reviewer.
Helpers
-------
* helpers_test.go waitFor(t, d, msg, cond) centralises the
10ms-poll-until-deadline pattern that previously appeared four
times across stream_test / renew_test / reconnect_test.
* TestFakeCaller_QueueThenDefaultFallback: self-test for the fake.
When the fake grows to 100+ LOC, debugging a flaky stream test
should not also require investigating whether the fake itself
behaves correctly.
The previous 30-second cap meant a 1000-camera fleet recovering from
a switch reboot would generate a sustained 33 RPS of doomed
CreatePullPointSubscription traffic against still-booting cameras,
and the synchronised retries would arrive in phase. Two changes:
* Cap raised to 5 minutes. Single-camera recovery latency goes from
'<=30s after camera comes back' to '<=300s', which is fine because
by the time we are this deep in backoff the camera has already been
unreachable through 6+ attempts (1s, 2s, 4s, 8s, 16s, 30s under the
old cap) — the marginal recovery delay is acceptable to avoid the
network melt.
* Symmetric ±25% jitter on every recreate sleep so synchronised
drops (switch reboot, DHCP storm, NTP slew) do not cause
synchronised reconnect surges. Standard practice — same shape AWS,
Cloudflare and HA event_manager use.
Tests assert the jitter range, the documented cap value (so a future
maintainer flipping it back to 30s notices in CI), and that jitter
varies across calls (proves the rand source is wired).
API-shape changes flagged as 'hard to reverse after v1' by the
architect reviewer. Acceptable to do now while no external code
imports the package; would be breaking later.
Surface tightening
------------------
* Decode unexported to decode. The Stream is the only intended caller;
exposing the helper invited future API drift. Same-package tests
still reach it.
* TopicFilter renamed to RawTopicFilter to signal that the value is
fed verbatim into the SOAP envelope and is the 'advanced escape
hatch', not the supported routing surface. Callers should normally
leave it empty and rely on Classify.
Options zero-value policy clarified
-----------------------------------
* Field godoc on every numeric option now explicitly states 'zero
means default' so the policy is local, not buried in
withDefaults().
* New DisableReconnect bool — addresses the
ReconnectAfterFailures=0-as-disable footgun the API reviewer flagged.
Reader can no longer confuse 'unset, fallback to default' with 'opt
out of reconnect'.
* BufferSize semantics extended: zero -> default (16), negative ->
unbuffered (0), positive -> explicit size. Lets callers ask for
back-pressure-only channels.
Default tuning
--------------
* MessageLimit default raised from 10 to 32. Busy AXIS cameras with
several configured inputs / analytics rules can burst beyond 10
per pull; the lower cap meant up to one PullTimeout of added
latency for the queued overflow without saving anything
meaningful. 32 covers observed bursts with no real overhead on
quiet pulls.
Caller interface
----------------
* Doc comment now states the goroutine-safety contract Stream
depends on (pull loop and renew loop call from separate
goroutines). *onvif.Device satisfies it via http.Client.
Package documentation
---------------------
* doc.go rewritten as a real godoc landing page: usage snippet,
invariants (channel close, Close idempotency, NewStream does I/O,
buffer semantics), reconnect behaviour and AfterReconnect, and a
pointer to topics.go for the classifier table. Replaces the
earlier stub that referenced unimplemented identifiers.
Replaces the bare fmt.Errorf wrappers on the Errors channel with three
typed errors and adds an Event.AfterReconnect flag so consumers can
distinguish post-recreate replay events from live ones.
Typed errors
------------
ErrPullFailed, ErrRenewFailed, ErrRecreateFailed all implement
Unwrap() and Op() Op. Consumers can branch with errors.As without
parsing strings:
var pull ErrPullFailed
if errors.As(e, &pull) { /* transient; logged */ }
var recreate ErrRecreateFailed
if errors.As(e, &recreate) { /* alert: camera may be offline */ }
Op() returns OpPull / OpRenew / OpRecreate for cases where the caller
wants to log the operation name without unwrapping. Both addressed
the review's 'highest-leverage v1 change' concern about bare error on
the Errors channel.
AfterReconnect observability
----------------------------
ONVIF cameras replay each property's current value with
PropertyInitialized whenever a new pull-point subscription is
established (per the Event Service spec). A consumer doing edge
detection on motion = StateActive would otherwise see a phantom
'motion started' for every active property after every reconnect.
The pull loop now tracks an afterReconnect flag local to the
goroutine: set to true when attemptRecreate returns justRecreated,
applied to every emitted event, cleared on the first non-Initialized
event we see. This bounds the replay window naturally — once the
camera has finished sending current state, the next event tells us
we're live.
attemptRecreate now returns (justRecreated, cont) so the pull loop
knows whether the just-completed recreate succeeded vs. the call
returning due to ctx-cancel during backoff.
Test coverage
-------------
* errors_test.go: typed-error Unwrap/Op assertions plus
Stream-level proof that pull and recreate failures arrive on the
Errors channel wearing the right type.
* AfterReconnect flag: drives the stream through a failure, observes
the next event carries the flag and the one after does not.
Addresses the five ship-blocker findings from the second review:
1. Bounded body read (review F1 / R-HIGH)
readClose now wraps resp.Body with io.LimitReader(10 MiB). A
hostile or buggy camera streaming an unbounded body cannot OOM
the agent. Legitimate PullMessages payloads are <200KB even with
dense analytics.
2. SOAP Fault detection (review F3)
unmarshalNode now scans for SOAP 1.1 faultstring and SOAP 1.2
Reason/Text BEFORE the missing-element error path. Auth failures
('not authorized'), InvalidFilterFault and expired-subscription
faults now surface their reason text instead of collapsing to
the unhelpful 'response missing PullMessagesResponse element'.
This is the difference between a debuggable error and a hidden
one when a customer's credentials change.
3. Absolute Renew TerminationTime (review F1 wire-correctness)
renewPullPoint now sends an RFC3339 UTC datetime
('2026-05-21T10:30:00Z') instead of a relative xsd:duration
('PT60S'). WS-BaseNotification §6.1.1 accepts both, but older
Hikvision, some Dahua and Bosch firmwares only accept the
absolute form — the library's own type comment even flags this
('BUG(r) Bad AbsoluteOrRelativeTimeType type').
4. Bounded Close (review P0)
Close now wraps Unsubscribe in a 5s timeout. Previously a
TCP-accepted-but-never-replying camera would wedge Close
indefinitely; now Close returns with a timeout error and the
subscription expires on its own at InitialTermination.
5. Explicit channel-close ordering after wg.Wait
The run goroutine previously relied on defer-LIFO to guarantee
renew exits before close(errors). Future maintainers extending
run() could invert that order silently. Closes are now explicit
sequential statements after wg.Wait() so the invariant is
local, not order-of-defers magic.
Also expands wsnt:UtcTime parsing in decode.go to cover the four
formats observed across vendor firmwares: RFC3339 with sub-seconds,
compact offsets ('+0200', Geovision/Dahua), and naked timestamps
without timezone (older Hikvision; per spec UTC is implied).
Caller interface gains a doc comment noting it must be safe for
concurrent use, documenting the contract Stream depends on (*onvif.
Device satisfies it via http.Client).
Tests added: SOAP 1.1 and 1.2 fault extraction, fault surfacing
through unmarshalNode, Renew absolute-datetime assertion,
Close-with-blocked-Unsubscribe returning within the timeout. -race
clean.
gofmt -w pass on reconnect_test.go. The Options struct field names had
mismatched alignment; reformatted to match gofmt canonical layout. No
behaviour change.
Adds automatic CreatePullPointSubscription recreation when the pull
loop hits ReconnectAfterFailures (default 3) consecutive errors.
Mirrors what production ONVIF clients (Home Assistant event_manager,
Milestone integration) do because pull points die for many reasons
none of which surface as a clean SOAP fault: camera reboot, NAT
session timeout, subscription garbage-collected after a renew miss,
firmware bug. Recreating is the only reliable recovery; Renew alone
cannot save an already-dropped subscription.
Two new options
---------------
* ReconnectAfterFailures int (default 3) — how many consecutive pull
failures trigger recreate. Conservative default; tunable for
always-on cameras vs flaky NAT.
* RetryBackoff time.Duration (default 1s) — base sleep between pull
retries; recreate failures double this up to a 30s cap so a
permanently broken camera does not hammer the network.
Lifecycle changes
-----------------
* Stream.pullPoint is now mutex-protected — the renew goroutine reads
it concurrently with the pull loop installing a new address after
recreate. getPullPoint/setPullPoint accessors keep the locking
contained.
* On successful recreate, failure count and backoff reset to defaults
so the loop is back to its happy-path cadence.
* On recreate failure, the loop continues retrying (until ctx cancel)
with exponentially increasing sleep — never blocks Close.
Tests cover: post-failure recreate hits a different SubscriptionRef
Address and subsequent events come from the new endpoint; exponential
backoff drives multiple recreate attempts when the camera stays down;
defaults match production-sensible 3 failures / 1s backoff. -race
clean.
Adds a background renew loop alongside the pull loop. ONVIF pull-point
subscriptions expire at the InitialTerminationTime supplied to Create;
without periodic Renew calls the camera silently drops the
subscription and subsequent pulls start returning empty messages — the
shape the existing agent's heartbeat code in cloud/Cloud.go has been
papering over by occasionally recreating subscriptions.
Design
------
* New Options.RenewMargin (default 10s) — how far before
InitialTermination expiry the renew fires. Smaller margins mean
fewer SOAP round-trips; larger margins tolerate slow networks. With
default 60s termination + 10s margin we renew every 50s, which is in
line with what production NVRs (Milestone, Genetec) use.
* The renew loop runs in a separate goroutine sharing ctx with the
pull loop. WaitGroup synchronisation in run() ensures both have
exited before close()-of-channels happens, so a renew in flight
during Close() cannot send on a closed Errors channel.
* Pathological config (RenewMargin >= InitialTermination) falls back
to renewing at termination/2 rather than busy-looping or never
renewing.
* renewPullPoint sends a wsnt:Renew SOAP against the SubscriptionRef
Address with the same InitialTermination duration; renew errors
surface on Errors non-blockingly, identically to pull errors.
Tests use very short termination/margin (80-100ms / 10ms) so a single
test run observes multiple renews within ~500ms, and assert that
renew calls target the SubscriptionReference endpoint (not the device
endpoint). -race clean.
Introduces Stream, the typed event consumer the package will eventually
present to callers, plus the caller seam needed to test it without
hitting a real camera.
Stream owns one ONVIF pull-point subscription end-to-end:
* CreatePullPointSubscription on construction so authentication and
reachability problems surface synchronously from NewStream rather
than landing on the Errors channel after the goroutine starts.
* Background pull loop calls PullMessages against the
SubscriptionReference Address returned by Create. Each
NotificationMessage is fed through Decode and pushed on the Events
channel, with context cancellation honoured between every step so a
Close cannot get stuck behind a long-server-side-wait pull.
* Errors during a pull are surfaced on a separate Errors channel using
a non-blocking send; a stalled consumer drops older errors instead
of blocking the loop. The loop sleeps briefly (ctx-aware) and
retries — automatic subscription recreation lands in the
reconnect-on-error commit.
* Close cancels the context, waits for the run goroutine to exit,
Unsubscribes the pull point and closes Events/Errors. sync.Once
keeps it idempotent.
Design seams
------------
* caller interface (CallMethod + SendSoap) abstracts *onvif.Device so
tests can substitute fakeCaller without an HTTP server. deviceCaller
is the production adapter; newStream takes the interface, NewStream
takes the concrete *onvif.Device. The same shape lets a future commit
add WithClassifier / WithClock / WithCaller options if the architect
reviewer's pluggable-classifier note becomes urgent.
* now func() time.Time is a Stream field so a future clock-injecting
test (renew timing, observed-at determinism) can swap it.
* unmarshalNode keys on the local XML name, sidestepping namespace
matching since SOAP envelopes from different vendors prefix the
PullMessagesResponse and CreatePullPointSubscriptionResponse with
arbitrary tev:/tev1:/... bindings. This is the same trick the agent's
getXMLNode used; lifting it here lets the agent eventually drop its
copy.
Options and defaults
--------------------
PullTimeout 5s, MessageLimit 10, InitialTermination 60s, BufferSize 16
match what the existing agent code uses. TopicFilter defaults to empty
so AXIS cameras work out of the box — the verified topic table is
intentionally the routing layer, not a server-side filter, because the
agent will frequently want digital I/O and motion on the same stream.
Tests cover the create-then-pull-then-close happy path, that pulls
target the SubscriptionReference Address (not the device endpoint),
construction failure on CreatePullPoint error, context-cancel exits
the loop cleanly with channels closed, transient pull errors land on
Errors without stopping decode of subsequent good messages, idempotent
Close, and Options default values. -race clean.
Adds the Decode entry point that converts the ONVIF wire form into the
package's typed Event. The agent (and any other consumer) no longer has
to walk NotificationMessage.Message.Message.Data.SimpleItem chains and
hand-special-case per-vendor data item names.
Decoding rules
--------------
* Topic -> Kind via the verified Classify table.
* PropertyOperation parses Initialized/Changed/Deleted; absent or
unrecognised -> PropertyUnknown (the attribute is optional per
WS-Notification).
* UtcTime parses RFC3339Nano first, RFC3339 second, normalised to UTC.
Absent or unparseable -> DeviceTime is zero. Camera clocks drift; the
type doc already steers callers to prefer Timestamp.
* State extraction scans Data items in order for the first boolean-like
value (true/false/1/0/active/inactive, case-insensitive). This handles
every vendor data item in the verified table — IsMotion, State,
IsTamper, LogicalState, active, Motion, triggered, SoundDetection,
TamperingDetection — without a per-kind switch.
* Edge-triggered topics (LineDetector/Crossed with only ObjectId) yield
StateUnknown, matching the topic-rule doc note.
* Source and Data are full ONVIF SimpleItem name->value maps so callers
retain multi-item info (AXIS AOA active+classType+confidence, digital
I/O InputToken+LogicalState, analytics VideoSourceConfigurationToken+
Rule). Empty notifications yield nil maps, matching the Event
zero-value contract from types_test.go.
* Topic, Source and Data are always populated even when Kind is
KindUnknown, so consumers can log/route unclassified events.
Tests cover the AXIS motion happy path, the inactive case, the Hanwha
numeric-string variant, the Avigilon 'active' literal, multi-item AOA
decode, the LineDetector edge-trigger semantic, unknown-topic wire
preservation, every PropertyOperation literal, RFC3339 with sub-second
and timezone offsets, and case-insensitive State extraction.
Parallel expert review of the four-commit scaffold surfaced 13 actionable
items split across API design, ONVIF domain accuracy, Go idiomaticity and
test rigor. This change addresses them before the Stream type lands, when
the public surface is still cheap to move.
API shape (hard-to-reverse before tagging)
------------------------------------------
* Rename EventKind -> Kind and EventState -> State to avoid the
stream.EventKind / stream.EventState stutter when imported.
* Restructure Event for non-lossy decode:
- Source string and RawValue string replaced with Source/Data maps so
multi-item ONVIF Source and Data lists (e.g. AXIS AOA emitting
active+classType+confidence; DigitalInput carrying InputToken+
LogicalState) are preserved.
- Add DeviceID so a single channel can fan in events from multiple
cameras.
- Add DeviceTime parsed from wsnt:UtcTime alongside the local
observation Timestamp. The earlier doc-comment decision to bake-in
'drop UtcTime' was a policy disguised as an API; expose both and let
callers choose.
Classifier accuracy (ONVIF domain audit)
----------------------------------------
* Introduce KindImageQuality for tns1:VideoSource/ImageTooDark|Bright|
Blurry. These are imaging-quality alarms that integrators route
separately because they fire on sunset/dawn/condensation, not tamper.
Previously mis-classified as KindTampering.
* Add tns1:VideoSource/GlobalSceneChange -> KindTampering, which is the
real lens-cover signal on firmwares without TamperDetector.
* Anchor the TamperDetector rule to 'TamperDetector/Tamper' so a
hypothetical 'TamperDetectorLog' path cannot match.
* Narrow MyRuleDetector from container-match to an explicit whitelist
(HumanDetect, VehicleDetect, PeopleDetect, ObjectsInside, FaceDetect).
Bosch publishes Counter and Occupancy under MyRuleDetector too; those
must not classify as ObjectDetected.
* Add the AXIS Guard suite (MotionGuard, FenceGuard, LoiteringGuard) ->
KindMotion. Common on AXIS deployments configured with these apps
instead of basic VMD.
* Drop the bogus Device1ScenarioANY test fixture; AOA uses numeric
scenarios (Device1Scenario1, Device1Scenario2). The 'ANY' suffix was a
borrow from the older Guard suite's Camera1ProfileANY pattern.
* Document the edge-trigger semantics of LineDetector/Crossed in the rule
comment so decoder consumers do not expect a State boolean.
Tests
-----
* String tests now use t.Run subtests so failures name the case.
* TestKindStringsAreUnique guards against accidental String() aliasing
when adding new kinds.
* TestEventFieldAssignmentRoundTrip exercises the new field set
including DeviceID, Source/Data maps and DeviceTime.
* Canonicalisation table now covers: double slash, colon-only segment,
trailing colon, multi-colon-in-segment, leading/trailing slash,
no-colon passthrough. Locks the actual behaviour so future refactors
see regressions.
* False-positive negatives: Counter and Occupancy under MyRuleDetector,
AudioEncoderConfiguration, RelayFailure, DigitalInputConfiguration,
TamperDetectorLog, MotionRecording/Started — all assert KindUnknown.
* TestClassifyRuleOrder_ObjectAnalyticsBeforeGenericObjects pins the
ordering invariant called out by the architect reviewer.
Documentation
-------------
* doc.go trimmed so it does not advertise NewStream / Events / Errors /
Close before those identifiers exist — the godoc reader will no longer
see dead names. Re-expanded when the Stream type lands.
Deferred to the Stream commit
-----------------------------
* PropertyUnknown vs PropertyUnset disambiguation — kept as
PropertyUnknown for now with a clarified doc comment; revisit when the
decoder needs to distinguish 'absent on wire' from 'unparseable'.
* Classifier pluggability (WithClassifier option) — meaningful only once
there is a Stream; revisit at that commit.
The rest of github.com/kerberos-io/onvif uses stretchr/testify (assert,
require) consistently — Device_test.go, event/type_test.go,
media2/types_test.go, ws-discovery/networking_test.go. Migrate the two
new test files in event/stream from stdlib t.Errorf to the same testify
convention so the package fits in without local style variation.
No production-code change; no behaviour change.
Cross-checked each topic string against public sources before adding the
rule, and inlined the citation next to the rule it supports so future
maintainers can audit the table:
* Hikvision motion: CellMotionDetector/Motion (Hikvision PDF on third
party motion troubleshooting) plus the VideoSource/MotionAlarm
fallback emitted by newer firmware.
* Hikvision tamper-class scene change: VideoSource/ImageTooDark|Bright|
Blurry — present in the ONVIF topic namespace; treated as Tampering
for routing.
* Bosch motion: VideoAnalytics/MotionAlarm (Bosch metadata/IVA PDF) —
NOT VideoSource/MotionAlarm. The earlier 'tnsbosch:MotionAlarm' guess
in PR #194 was wrong; Bosch uses standard tns1 namespace under
VideoAnalytics.
* Hanwha (Samsung Wisenet): VideoAnalytics/tnssamsung:MotionDetection,
VideoAnalytics/tnssamsung:TamperingDetection,
AudioAnalytics/tnssamsung:SoundDetection — confirmed via HA #66493
capture.
* Avigilon: per-segment-namespaced serialisation
(tns1:Device/tns1:Trigger/tns1:Relay) folded by canonicalization.
Documented in Avigilon's own ONVIF subscription guide.
* Object analytics: LineDetector/Crossed, FieldDetector/ObjectsInside
and the MyRuleDetector container for vendor rule names (Bosch IVA,
Dahua SMD) — sourced from ONVIF Analytics Service Spec v22.06.
* AXIS Object Analytics: prefix match on ObjectAnalytics/ to absorb
the dynamic Device1Scenario<N> suffixes (AXIS counting-data docs).
Empirical topic table cross-checked with openvideolibs/onvif-parsers
(Apache-2.0), the package the Home Assistant ONVIF integration imports —
referenced from the package doc-comment.
Test cases now cover the verified topic for every supported vendor plus
case-sensitivity and canonicalization. No code change for callers: the
public API is still just Classify(topic) -> EventKind.
Adds a Classify function that maps ONVIF topic strings to EventKind so the
agent does not need to know AXIS vs Hikvision vs Bosch topic conventions.
The classifier canonicalizes topics by stripping XML-namespace prefixes from
each path segment, which collapses vendor variants like
'tns1:Device/tnssamsung:DigitalInput' and 'tns1:Device/Trigger/DigitalInput'
to a single matchable form.
Motion coverage on day one:
* tns1:VideoSource/MotionAlarm (AXIS, Bosch, Dahua, ...)
* tns1:VideoAnalytics/<vendor>:MotionAlarm
* tns1:RuleEngine/CellMotionDetector/Motion (ONVIF standard, Hikvision)
* tns1:RuleEngine/MotionRegionDetector/Motion (AXIS region rule)
* tnsaxis:CameraApplicationPlatform/ObjectAnalytics/...
Also covers Tamper, DigitalInput, Relay (DigitalOutput), object analytics
and audio alarms, so the same Stream can replace the agent's ad-hoc digital
I/O polling without losing coverage.
Introduces a new event/stream sub-package that will host the long-running,
channel-based consumer for ONVIF device events. This commit only lays down
the value types — EventKind, EventState, PropertyOperation and the Event
struct — together with Stringer methods and zero-value tests.
The intent is to give callers a vendor-neutral surface (Motion, DigitalInput,
etc.) so they do not need to special-case AXIS, Hikvision, Avigilon, Hanwha,
Bosch or Dahua topic strings. Decoding, topic classification and the Stream
type itself land in follow-up commits.
