Add a setting blacklistExpiryTime to expire blacklisted service locations#4983
Add a setting blacklistExpiryTime to expire blacklisted service locations#4983robertbryer wants to merge 8 commits into
Conversation
… after a failure; refactor ContentSteering TTL to use blacklistController expiry
There was a problem hiding this comment.
Pull request overview
This PR adds an expiry mechanism for blacklisted service locations (via a new streaming.blacklistExpiryTime setting) and refactors Content Steering TTL-based blacklist expiry to use the same BlacklistController logic, enabling long-lived streams to eventually retry previously-failed locations.
Changes:
- Add
streaming.blacklistExpiryTime(seconds) to player settings + TypeScript typings, defaulting to-1(disabled). - Refactor
BlacklistControllerto store blacklist entries with optional expiry timers and support Content Steering TTL-driven expiry viasetContentSteeringBlacklistExpiry. - Update Content Steering handling to set blacklist expiry from
CONTENT_STEERING_REQUEST_COMPLETEDTTL and add unit tests using fake timers.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/unit/test/streaming/streaming.controllers.BlacklistController.js | Adds unit tests validating expiry behavior (Content Steering TTL and settings-driven). |
| src/streaming/utils/baseUrlResolution/ContentSteeringSelector.js | Sets blacklist expiry from Content Steering TTL on request completion; removes old per-entry timeout logic. |
| src/streaming/utils/BaseURLSelector.js | Removes call to contentSteeringSelector.reset() since expiry is now handled by BlacklistController. |
| src/streaming/controllers/BlacklistController.js | Implements timed expiry for blacklist entries; adds Content Steering TTL override support. |
| src/core/Settings.js | Documents and adds default value for streaming.blacklistExpiryTime. |
| index.d.ts | Exposes blacklistExpiryTime in the public settings typings. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
I see a problem with adding the blacklist expiry logic to the BlacklistController. We also use BlacklistController in Stream.js and LocationSelector.js. While the latter also uses the concept of a service location the BlacklistController in Stream.js is used for the purpose of ignoring corrupt segments. In that case the blacklistExpiryTime should not be taken into account.
I think we need to clarify:
- Should the expire time logic also be applied for
Locationelements or only forBaseURLelements? - How do we handle the case of blacklisting segments. Ideally we would have one parent class
BlacklistControllerand then a child class that inherits the common functionality and extends it with the expiry timer
|
@robertbryer Friendly reminder, please take a look at my review comments |
|
Sorry for the delay – I was off on holiday. I agree about not doing this for the segments; I wouldn't expect those to be repaired after a time. I've amended it to do that and I just did this with a config switch, but let me know if you would prefer it refactored to be a subclass. For the location, I'm completely at a loss how it's triggered. I can't find a direct call to I think locations failing for network reasons could reasonably be covered by the same expiry following the same behaviour of not failing back proactively. Would it switch location for any other reason (manifest parse errors or similar?) |
dsilhavy
left a comment
There was a problem hiding this comment.
Thanks for the changes @robertbryer , two minor remaining comments.
I agree about not doing this for the segments; I wouldn't expect those to be repaired after a time. I've amended it to do that and I just did this with a config switch, but let me know if you would prefer it refactored to be a subclass.
Your approach looks good to me
For the location, I'm completely at a loss how it's triggered. I can't find a direct call to .add(); and the event SERVICE_LOCATION_LOCATION_BLACKLIST_ADD does not appear to be triggered anywhere. Is this broken?
I agree, it looks like the event is never triggered I need to check that.
I think locations failing for network reasons could reasonably be covered by the same expiry following the same behaviour of not failing back proactively. Would it switch location for any other reason (manifest parse errors or similar?)
Agreed we can use the same logic here.
… controller so it picks up a 429 response ttl
|
I've made the changes to just pull straight from the ContentSteeringController.js, so it should always be getting the ttl now. I also noticed that the 429 error path was not taking the retry-after header, and included a fix for that. |
Adds a setting
streaming.blacklistExpiryTime, a time in seconds after which a service location is removed from the blacklist. This would allow for example, the first service location to be returned to after the second service location fails, which can be useful when running a very long-lived stream. It will not actively switch back until a failure on the second service location.It also refactors the existing expiry that uses Content Steering's TTL value to use the same BlacklistController expiry.