Skip to content

fix(event/stream): scale the close drain bound with PullTimeout - #10

Open
ttradesman wants to merge 2 commits into
kerberos-io:masterfrom
sharedjourney:fix/close-drain-outlasts-pull
Open

fix(event/stream): scale the close drain bound with PullTimeout#10
ttradesman wants to merge 2 commits into
kerberos-io:masterfrom
sharedjourney:fix/close-drain-outlasts-pull

Conversation

@ttradesman

@ttradesman ttradesman commented Jul 23, 2026

Copy link
Copy Markdown

Scale the close drain bound with PullTimeout

Summary

closeDrainTimeout was a fixed 5 seconds. Close uses it to wait for the pull and renew goroutines to exit, but those loops block inside caller.SendSoap, which is not ctx-aware — as the caller interface documents, only the HTTP client's own timeout can unblock them. So the drain bound has to outlast an in-flight poll, and a fixed 5s only did that while PullTimeout was also 5s.

Why it matters

Any caller raising PullTimeout — which is the natural thing to do, since a long-poll is meant to be long and a longer poll means fewer requests — makes the drain the shorter of the two. Close then times out on every shutdown that lands mid-poll, which for a 30-second poll is most of them.

The consequence is not just a spurious error. When the drain times out, Close returns early and the Unsubscribe below it is skipped:

select {
case <-s.done:
case <-time.After(closeDrainTimeout):
    s.closeErr = fmt.Errorf("close: pull/renew loops did not drain within %s ...", closeDrainTimeout)
    return          // <- Unsubscribe never runs
}

So the pull-point is left registered at the camera until InitialTermination expires on its own. On a consumer that rebuilds its stream on reconfiguration or reconnect, that orphans a subscription per cycle, and the goroutine and its socket stay live until the HTTP call eventually returns.

Changes

The default is now derived rather than fixed, from whichever of the two relevant timeouts is longer:

d.CloseDrainTimeout = max(PullTimeout, dev.HttpClient.Timeout) + closeDrainSlack   // closeDrainSlack = 10s

Both matter, and taking only the first is not enough. PullTimeout is how long the camera holds a poll, but http.Client.Timeout is what actually caps the request — it has to cover dial, TLS and the response transfer on top of the poll it outlasts, so callers set it higher. A 30s poll behind a 40s ceiling would otherwise get a 40s drain against a call that can itself run 40s: a dead heat, and Close still loses the shutdowns that land mid-poll.

A zero client ceiling means unbounded, where no finite drain helps, so PullTimeout remains the best bound available in that case.

This is derived in NewStream, because that is the only entry point with the device in hand. Callers wiring their own caller implementation keep the PullTimeout-only bound. Options.CloseDrainTimeout overrides both, following the package's existing zero-means-default convention.

closeDrainTimeout the constant is replaced by closeDrainSlack, which is the part that was genuinely a constant; the bound itself was never one.

Tests

TestCloseDrainTimeout_ScalesWithPullTimeout covers the derivation from the default poll, derivation from a long poll, and an explicit value overriding both. It also asserts the invariant directly — the resolved drain must exceed the resolved PullTimeout — so a future change to either default cannot silently reintroduce the inversion.

TestDrainFor_BoundedByTheLongestPossibleCall covers the client-ceiling half: a ceiling above the poll becomes the bound, a ceiling below it does not, equal values leave the poll as the bound, and an unbounded ceiling falls back to the poll. Each case also asserts the resulting drain outlasts both inputs.

TestClose_BoundedWhenLoopsStuckOnHungHTTP now sets CloseDrainTimeout explicitly. It had been relying on the constant being small, and would otherwise sit for the derived default on every run.

Compatibility

Behavioural change for callers that set a PullTimeout above 5 seconds: Close now waits longer before giving up, which is the point — it waits long enough to drain cleanly and reach Unsubscribe. Callers who preferred the old fixed bound can set CloseDrainTimeout: 5 * time.Second and get exactly the previous behaviour. No signature changes; Options gains one optional field.

@cedricve

cedricve commented Aug 6, 2026

Copy link
Copy Markdown
Member

@ttradesman we have some merge conflicts now, due to merging your previous PR's. All are currently deployed in v3.9.0. Many thanks!

closeDrainTimeout was a fixed 5s while the pull loop can be parked
inside a non-ctx-aware SendSoap for as long as the caller's HTTP client
allows. That was fine when PullTimeout was also 5s, but a caller raising
the poll made the drain the shorter of the two: Close then times out on
every shutdown landing mid-poll, and the Unsubscribe below it is
skipped, orphaning the pull-point at the camera until its termination
expires.

Derive the default as PullTimeout + closeDrainSlack so it outlasts a
poll, and expose CloseDrainTimeout for callers whose client ceiling is
higher still — that ceiling, not PullTimeout, is the real worst case
for a stalled camera, and only the caller knows it.

The hung-HTTP test now sets it explicitly; it was relying on the
constant being small.
The drain default was PullTimeout + slack, which bounds the poll but
not the call. PullTimeout is how long the camera holds a poll;
http.Client.Timeout is what actually caps the request, and callers set
it higher — it has to cover dial, TLS and the response transfer on top
of the poll it outlasts.

So a caller with a 30s poll and a 40s ceiling got a 40s drain against a
call that can also run 40s: a dead heat, and Close still times out and
skips Unsubscribe on the shutdowns that land mid-poll. The Options
escape hatch covered it, but a default that every caller has to
override is not much of a default.

Take whichever of the two is longer. A zero ceiling means unbounded,
where no finite drain helps, so the poll stays the best bound available.

Derived in NewStream because only that entry point sees the device;
newStream keeps the PullTimeout-only bound for callers wiring their own
caller implementation.
@ttradesman
ttradesman force-pushed the fix/close-drain-outlasts-pull branch from 11f72c5 to 9d56f10 Compare August 7, 2026 12:36
@ttradesman

Copy link
Copy Markdown
Author

@ttradesman we have some merge conflicts now, due to merging your previous PR's. All are currently deployed in v3.9.0. Many thanks!

Fixed! 🫡

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants