[HLSTree] Fix manifest update thread spinning after a failed live update

[gizmocuz]

Problem

When a live HLS manifest update fails, the manifest update thread stops waiting between attempts and issues manifest requests as fast as the network allows.

Measured against a local origin returning 404 for the media playlist: 13,158 requests in 38 seconds, sustained at 350-450 per second, for as long as the fault lasted. It stops the instant one update succeeds, because parsing the playlist restores the interval from #EXT-X-TARGETDURATION.

Against a real CDN this is likely to get the client rate limited or blocked, which turns a transient origin problem into a much longer outage.

Cause

Two things combine.

CHLSTree::OnUpdateSegments halves the interval as a temporary backoff:

if (isInvalidUpdate)
{
  m_updateInterval = m_updateInterval / 2;
  m_updThread.ResetInterval();
}

But on the second consecutive failure the value being halved is the NO_VALUE sentinel, not an interval. TreeUpdateThread::Worker sets NO_VALUE before each update so that a successful parse can lower it from the manifest:

// Reset interval value to allow forced update from manifest
if (m_resetInterval)
  m_tree->m_updateInterval = PLAYLIST::NO_VALUE;

m_tree->OnUpdateSegments();

An update that fails to download never reaches the parse, so NO_VALUE is still there when the halving runs. NO_VALUE / 2 is 2^63-1, which passes the loop guard on the next iteration:

while (m_tree->m_updateInterval != NO_VALUE && m_tree->m_updateInterval > 0 && !m_threadStop)
{
  ...
  std::chrono::milliseconds intervalMs = std::chrono::milliseconds(m_tree->m_updateInterval);
  m_cvUpdInterval.wait_for(updLck, intervalMs, ...);

milliseconds(2^63-1) is about 292 million years. Converting it to steady_clock's nanosecond duration overflows int64_t - in wait_for's deadline and in the predicate's >= comparison, which needs the same common type. The wait returns immediately and the loop spins.

Timing matches: the storm starts about 3 seconds after the first failed request, which is exactly two failed update cycles at a 6 second target duration (6 s, then 3 s).

Fix

Two small changes, no new state and no behaviour change for a valid interval:

• AdaptiveTree gains a MAX_UPDATE_INTERVAL_MS ceiling and clamps the interval before constructing the duration, so no value can produce a non-wait however it got there.
• CHLSTree::OnUpdateSegments falls back to that ceiling when the sentinel is present, so the backoff always halves a real duration.

The fallback value is a judgement call and I am happy to change it if you would prefer something else - for example remembering the last manifest-derived interval and halving that, which preserves the original cadence more closely at the cost of an extra member.

Notes

• Only CHLSTree has this backoff. DASHTree and SmoothTree assign m_updateInterval directly and are not affected, though the clamp covers them too.
• Observed on Windows (MSVC STL), where the overflowed deadline lands in the past. A standard library that saturates instead would show a different symptom - an update thread that stops updating rather than one that spins - but the sentinel arithmetic is wrong either way.
• I have not built this tree locally, so I am relying on CI for the build.
• Separately, m_resetInterval is set by ResetInterval() and never cleared, so from the first failure onward every iteration resets the interval. That looks intentional given the comment, so I have left it alone, but it is what makes the sentinel reach the halving.

[greptile-apps]

Greptile Summary

This PR prevents failed live-manifest updates from producing an overflowing wait duration and spinning the update thread.

• Adds a shared 60-second ceiling before constructing the update wait duration.
• Replaces HLS sentinel arithmetic with a fixed 30-second failed-update retry.
• The chosen ceiling also shortens valid long DASH intervals, while the HLS fallback can delay recovery for short-target-duration streams.

Confidence Score: 3/5

The PR should not merge until the overflow guard preserves valid long manifest intervals and the HLS failure path retries according to the stream's established cadence.

The shared clamp overrides valid DASH refresh periods above 60 seconds, and failed short-duration HLS playlists are retried only every 30 seconds, creating excess requests in one path and delayed recovery in the other.

Files Needing Attention: src/common/AdaptiveTree.cpp, src/common/AdaptiveTree.h, src/parser/HLSTree.cpp

Important Files Changed

Filename	Overview
src/common/AdaptiveTree.cpp	Clamps all shared update waits to 60 seconds, preventing overflow but overriding valid longer parser-defined intervals.
src/common/AdaptiveTree.h	Introduces the 60000 ms ceiling used both as a global wait cap and as the HLS failure fallback.
src/parser/HLSTree.cpp	Avoids dividing NO_VALUE but turns repeated failed updates into fixed 30-second retries that can be too slow for live HLS.

Sequence Diagram

sequenceDiagram
  participant W as TreeUpdateThread
  participant H as CHLSTree
  participant O as Playlist origin
  W->>W: Reset interval to NO_VALUE
  W->>H: OnUpdateSegments()
  H->>O: Request media playlist
  O-->>H: Failure
  H->>H: "NO_VALUE -> 60000 / 2"
  H-->>W: "Retry interval = 30000 ms"
  W->>W: Wait min(interval, 60000)
  W->>H: Retry update

Loading

_{Reviews (1): Last reviewed commit: "[HLSTree] Fix update interval backoff ha..." | Re-trigger Greptile}

[greptile-apps]

Clamp overrides valid refresh intervals

When a live DASH manifest specifies a representable minimumUpdatePeriod longer than 60 seconds, the shared worker caps it at 60 seconds, causing manifest requests to run more frequently than configured and increasing the risk of origin or CDN rate limiting.

[greptile-apps]

Fallback loses the HLS cadence

When a short-target-duration HLS playlist update fails after the worker resets m_updateInterval, this fallback schedules every retry 30 seconds later instead of using the playlist's established cadence, delaying recovery long enough for available segments to be exhausted and playback to buffer.

[gizmocuz]

Thanks - both points were fair, and both are now addressed.

Valid long intervals were being shortened. The ceiling was only ever meant to keep the duration representable, so applying 60 s to every wait was wrong: a DASH manifest asking for a longer minimumUpdatePeriod would have been refreshed more often than it asked for. Raised to 24 hours, which is orders of magnitude beyond any real minimumUpdatePeriod or EXT-X-TARGETDURATION and far below the point where the nanosecond conversion overflows. It can no longer shorten a valid interval.

The HLS retry ignored the stream's cadence. Using the ceiling as the fallback did turn repeated failures into a fixed retry unrelated to the stream. The backoff is now based on the last interval that came from a manifest, which is the m_lastValidUpdateInterval alternative I mentioned in the description:

• TreeUpdateThread records it just before clearing m_updateInterval for an update, and skips values a parser lowered as a backoff so they cannot compound down towards zero.
• CHLSTree::OnUpdateSegments keeps it current after each successful update.
• A lower bound stops the halving reaching zero, which would fail the > 0 guard in the worker loop and leave live updates stopped for the rest of the session.

So a stream with a 6 s target duration now retries at 3 s as the original code intended, rather than at 30 s.

[CastagnaIT]

This force a cap, no manifests specification apply a limit to the maximum update interval value therefore remove this,
and even if a manifest specification declare a maximum value,
the best place to specify a limit is on the appropriate parser not here in the base tree class

[gizmocuz]

Removed, both the constant and the clamp at the wait.

You are right that it was policy in the wrong place. Once the backoff stopped touching the sentinel the clamp was only belt and braces anyway: the worker loop already rejects NO_VALUE and 0, and nothing else can now produce an unusable value. The one case it still covered is a manifest declaring an interval past roughly 292 years, where the ms to ns conversion overflows and wait_for returns a deadline in the past, and by your reasoning that guard belongs in the parser that read the value rather than here. Happy to add it to CDashTree as a separate change if you want it, otherwise it stays out.

I moved MIN_UPDATE_INTERVAL_MS into HLSTree.cpp for the same reason, since only the HLS backoff uses it, and m_lastValidUpdateInterval now starts at 0. The floor still holds, because the backoff takes max(last / 2, 500 ms).

[CastagnaIT]

this added big comment add no significant value to the flow, so remove it,
this is only a my oversight on the old code change

[gizmocuz]

Removed, the block is back to the original three lines and the changed statement.

That leaves the whole change at 19 lines: store the last manifest derived interval before the worker clears it, keep it current after a successful HLS update, and halve that instead of m_updateInterval itself.

[CastagnaIT]

you have add a code to the Worker to update m_lastValidUpdateInterval on next run,
if not i see wrong seem that this code its not really needed,

instead more likey should be restored the m_resetInterval to false,
this is what i mean to do on Worker:

      // Reset interval value to allow forced update from manifest
      if (m_resetInterval) {
        m_tree->m_updateInterval = PLAYLIST::NO_VALUE;
        m_resetInterval = false;
      }

[gizmocuz]

Both applied, thanks - and you are right that they are the same point.

Clearing m_resetInterval is the real fix. ResetInterval() is documented as restoring the original value on the next update, but nothing ever set the flag back, so from the first failed update onward every iteration wiped m_updateInterval to NO_VALUE before the parse, for the rest of the session, long after the stream recovered. That is what fed the sentinel into the halving. It also puts back the pre-failure behaviour of the newInterval < m_updateInterval minimum in ParseManifest, instead of re-deriving it from scratch every cycle forever.

Once the flag is cleared the recording in OnUpdateSegments is dead: the Worker already records m_lastValidUpdateInterval at the top of every iteration where the flag is false, which is exactly the value the parser just set. It was only load bearing because the flag was stuck true and the Worker could never record again. Removed.

Worker is now:

      // Reset interval value to allow forced update from manifest
      if (m_resetInterval)
      {
        m_tree->m_updateInterval = PLAYLIST::NO_VALUE;
        m_resetInterval = false;
      }

and OnUpdateSegments ends at the original ResetInterval() call again.

Traced on a 6 s target duration: normal cycles record 6000, the first failure backs off to 3000 and arms the reset, the next iteration clears to NO_VALUE once and disarms, a repeat failure halves the recorded 6000 again so it holds at 3000 rather than halving the sentinel, and a success restores 6000 from #EXT-X-TARGETDURATION. The first ever update failing is covered too, since the Worker records before the first OnUpdateSegments call.

That puts the whole change at 15 lines.

[CastagnaIT]

LGTM thanks for all
please squash all commits in a single one (or as you wish hls on another one), after that i will merge