fix(event/stream): reject a client timeout that cannot outlast PullTimeout

[ttradesman]

Reject a client timeout that cannot outlast PullTimeout

Summary

PullMessages is a long-poll: the client asks the camera to hold the connection open for up to PullTimeout and answer early if an event arrives. http.Client.Timeout is not symmetrical with it — it bounds the entire exchange, dial and TLS and request write and the wait for response headers, and it starts before the camera has even parsed the request. So a client ceiling equal to or below PullTimeout expires before the camera's hold does, on every interval with no event. Not intermittently: deterministically.

Nothing in the package noticed. NewStream accepted the configuration and returned a working Stream, and the caller had no way to learn that its polls could never succeed.

Why it matters

The failure is quiet rather than loud, which is what makes it worth guarding against rather than documenting. Because ReconnectAfterFailures recreates the subscription after three consecutive pull failures, and a fresh CreatePullPointSubscription makes the camera replay its whole property state, events keep arriving — so the stream looks alive.

Observed on a deployed camera with both values at 5s, over a ten-minute window:

11:39:25 fail  11:39:31 fail  11:39:37 fail → recovered (~90 events)
11:39:43 fail  11:39:49 fail  11:39:55 fail → recovered (~90 events)
11:40:00 fail  11:40:06 fail  11:40:12 fail → recovered (~90 events)

Ordinary polling never succeeded once. Every event reaching the consumer arrived on the reconnect cadence, roughly 18 seconds apart, in bursts of replayed subscription state rather than when the underlying thing actually happened. Roughly 540 of the 546 events in that window were the same property dump repeated seven times. The pull failures are logged at debug and the recreates succeed, so at default log levels this presents as a slow camera rather than a misconfiguration, and motion-triggered consumers see latency of up to a full reconnect cycle with no error anywhere.

Changes

NewStream now validates the device's client ceiling against the resolved PullTimeout before issuing the subscription call, and returns an error if it cannot outlast it.

Strict inequality is not sufficient. The client ceiling also has to cover dial, TLS and the response transfer on top of the poll it outlasts, which on a constrained link runs to hundreds of milliseconds — a ceiling one millisecond above PullTimeout loses the same race. The check therefore requires a real headroom floor, minClientHeadroom, currently 5s.

The error wraps a new ErrInvalidOptions sentinel. The existing ErrPullFailed / ErrRenewFailed / ErrRecreateFailed are transient and consumers retry them; this one never becomes valid by retrying, so a consumer that cannot distinguish it will loop forever on a configuration that can never work. The sentinel lets them short-circuit.

A zero client timeout is still accepted. It is what onvif.NewDevice fills in when a caller passes no client, so rejecting it would break every default consumer including this repository's own examples/event/stream. The comment no longer claims that case is safe, though — the caller interface documents that ctx cannot interrupt an in-flight SOAP call, so an unbounded client is precisely the case nothing can unwedge.

Tests

TestValidateClientTimeout covers the boundary in both directions: unbounded, comfortable headroom, exactly the minimum headroom, a hair under it, strictly-greater-but-no-headroom, equal values (the original bug), and a ceiling below the poll. TestErrInvalidOptions_IsDistinctFromStreamErrors pins that the sentinel does not match any of the three transient stream error types, which is the property consumers will branch on.

Compatibility

This is a behavioural change at construction: a configuration that previously produced a limping stream now fails fast. That is deliberate — the configuration can only ever fail — but it will surface for any existing consumer that sets a short client timeout without having noticed the consequence, so it warrants at least a minor version.

Options.PullTimeout and the package doc comment are updated to state the constraint.

Relationship to #10

These two changes share a root cause — PullTimeout is the clock every other timeout in the package has to respect, and nothing enforced that — but they are independent defects with different owners and different symptoms. This one concerns http.Client.Timeout, which the caller supplies, and breaks steady-state event delivery. #10 concerns the close-drain bound, which the package owns, and breaks shutdown by skipping Unsubscribe and orphaning the pull-point. Either can occur without the other: before this fix a consumer passing no client at all had an unbounded ceiling, so it had #10's bug and not this one.

They do overlap mechanically. Both extend NewStream, and both need to read the device's HTTP client timeout, so each branch carries its own small accessor for it. Whichever merges second will need a trivial rebase: delete the duplicate accessor and hoist the single read so it feeds both the validation here and the drain derivation there. Happy to do that rebase in whichever order suits you, or to fold the two into one PR if you would rather review them together.