Skip to content

feat(dvr): series recording improvements — immediate EPG scan, once-rule cleanup, defaults#1142

Open
Grimothy wants to merge 9 commits into
m3ue:experimentalfrom
Grimothy:DVR-Fixes
Open

feat(dvr): series recording improvements — immediate EPG scan, once-rule cleanup, defaults#1142
Grimothy wants to merge 9 commits into
m3ue:experimentalfrom
Grimothy:DVR-Fixes

Conversation

@Grimothy
Copy link
Copy Markdown
Contributor

Changes

Two feature commits on top of latest upstream/dev merge:

✨ Delete once-rules on completion and cancel, add series recording defaults (7a99acbe)

  • Once-rules are now cleaned up automatically after recording completes or is cancelled
  • Added default values for series recording configuration

⚡ Immediate EPG scan when series rule is created or re-enabled (df54776e)

  • Creating or re-enabling a series rule triggers an immediate EPG scan instead of waiting for the next scheduled scan
  • Reduces delay between user action and recording availability

Base

Merged with upstream/dev at 30f67c36.

Grimothy added 3 commits May 18, 2026 20:53
When a series rule is created or transitioned from disabled to enabled,
scheduleRuleImmediately() now scans the full EPG (configurable via
dvr.initial_lookahead_days, default 14 days) instead of the 30-minute
lookahead window used by the regular tick. Users now see upcoming
scheduled recordings immediately after creating a rule.

- Add DvrSchedulerService::scheduleRuleImmediately() that calls
  matchRule() with a 14-day lookahead
- Add DvrRecordingRule::created() hook to trigger immediate scheduling
  for enabled Series rules on creation
- Add DvrRecordingRule::saving() enabled-transition detection to trigger
  immediate scheduling when a rule goes from disabled → enabled
- Add dvr.initial_lookahead_days config (env DVR_INITIAL_LOOKAHEAD_DAYS,
  default 14)
- Add Pest tests for both creation and re-enablement scenarios
…ing defaults

- Delete once-rules after post-processing completes (not just disable)
- Delete once-rules on user cancel in DvrRecorderService::cancel()
- Delete once-rules in PostProcessDvrRecording early return for cancelled recordings
- Add default_series_mode and default_series_keep_last to DvrSetting
- Add Series Recording Defaults section to playlist DVR settings
- Use series defaults when creating rules via BrowseShows quick actions
- Add include_disabled_channels toggle to playlist DVR settings
- Extend EPG programme population lookahead to 30 days
- Add env() wrappers to config/cache, database, queue, session for local dev
- Revert broadcasting.php and reverb.php to original (remove APP_KEY fallback)
- Add reverb:start to composer dev script

This comment was marked as outdated.

@Grimothy
Copy link
Copy Markdown
Contributor Author

@sparkison --- what do you need me to fix?

@sparkison
Copy link
Copy Markdown
Member

sparkison commented May 21, 2026

@sparkison --- what do you need me to fix?

There are a few things that need to be addressed before this can merge, grouped by severity below.


🔴 Critical — Config File Regressions

These config files have intentionally hard-coded values to ensure correct behavior in the Docker setup. Making them env-readable allows users to inadvertently break things by setting (or not setting) .env values.

config/session.php — Revert both changes

- 'driver' => env('SESSION_DRIVER', 'redis'),
+ 'driver' => 'redis', // env('SESSION_DRIVER', 'database'),

- 'connection' => env('SESSION_CONNECTION', null),
+ 'connection' => 'session', // env('SESSION_CONNECTION'),

connection => 'session' was intentional — it targets the dedicated Redis session connection. Changing it to null causes sessions to fall back to the default Redis connection, which can cause key collisions with cache/queue operations. If a user sets SESSION_CONNECTION to an invalid value in .env, it will break silently.

config/queue.php — Revert

- 'default' => env('QUEUE_CONNECTION', 'redis'),
+ 'default' => 'redis', // env('QUEUE_CONNECTION', 'database'),

The inline comment explained why this was hard-coded (overriding Laravel's default database driver). Without it, a user setting QUEUE_CONNECTION=database will silently break the queue.

config/cache.php — Revert and restore the warning comment

- 'default' => env('CACHE_STORE', 'redis'),
+ // IMPORTANT: Redis is required for our cache store
+ // DO NOT change this to a different cache store
+ 'default' => 'redis',

The warning comment was the only thing preventing users from setting CACHE_STORE=file and breaking cache-dependent features.

config/database.php — Revert the SQLite path change

- 'database' => env('DB_DATABASE', database_path('database.sqlite')),
+ 'database' => database_path('database.sqlite'),

If a user has DB_DATABASE set for a non-SQLite connection (e.g. a database name like m3u_editor), the SQLite connection silently picks it up and tries to open storage/database/m3u_editor as a file path.


🔴 Critical — Bugs / Security (overlaps with Copilot review)

DvrCallbackController::isAuthorized() — Fail closed when token is empty

When proxy.m3u_proxy_token is empty/null, the method currently accepts all callbacks. Since this endpoint can transition recordings to Failed/PostProcessing, an unconfigured token makes the endpoint effectively open. Reject requests when the token is missing (or only allow open access in local/testing environments).

/api/dvr/callback in routes/web.php — CSRF 419

This route is behind the web middleware stack. The proxy's callback won't send a CSRF token so it will 419. Move it to routes/api.php or add it to the CSRF exception list.


🟡 Significant — Behavior / Correctness

"Cancel Recording" action dispatches StopDvrRecording (not cancel)

StopDvrRecording calls DvrRecorderService::stop(), which transitions to PostProcessing. A cancel should skip post-processing and mark the recording Cancelled. Either wire up the correct cancel path or rename the UI action to "Stop" to match what it actually does.

Debug log left in GuestBrowseShows::openShowDetail()

Log::info('GuestBrowseShows: openShowDetail called: '.$title);

This fires on every show detail open in production. Remove it.

config/dvr.phpDVR_MAX_ATTEMPTS_PER_AIRING doc/cast mismatch

The comment says null means unlimited retries, but the value is cast with (int) so it can never be null. Either remove the (int) cast and handle null explicitly in the scheduler, or update the comment (e.g. 0 = unlimited).

DvrSetting model — include_disabled_channels missing boolean cast

include_disabled_channels is added in the migration but not in the model's casts. Without it, the value comes back as 0/1 depending on the DB driver, which can cause subtle conditional bugs.

EpgApiController:185 — Incorrect boolean cast on vod param

// Before (broken — treats "false" string as truthy)
$vod = (bool) $request->get('vod', false);

// After
$vod = $request->boolean('vod');

🟢 Minor / Cleanup

Duplicate null assignment in M3uProxyStreamMonitor

$this->connectionError = null is assigned twice in a row. Remove the duplicate line.

HasGuestAuth::login() — Password not cleared after login

The original code explicitly cleared the password form field after a successful login. The PR replaces this with a redirect. Confirm the redirect fully clears the form state — if not, the password field could remain populated in the DOM.

docs/plans/dvr-feature.md committed

Planning/design docs tend to go stale and can mislead future contributors. Consider removing and preserving this in the PR description or a wiki page instead.

@sparkison
Copy link
Copy Markdown
Member

@Grimothy, I think once the config files are rolled back the tests will pass again too.

@Grimothy
Copy link
Copy Markdown
Contributor Author

Addressed review feedback and pushed updates to DVR-Fixes.

Fixes included:

  • ViewDvrRecording retry action now dispatches PostProcessDvrRecording using recording ID.
  • EpgApiController now parses vod with request boolean handling.
  • Removed duplicate connectionError assignment in M3uProxyStreamMonitor.
  • Added include_disabled_channels boolean cast in DvrSetting.

DVR include-disabled toggle is now honored end-to-end:

  • BrowseShows and GuestBrowseShows now respect include_disabled_channels for channel options and EPG scope filtering.
  • DvrScheduleTool now enforces the same behavior for direct scheduling paths:
    • schedule_once blocks disabled channels when toggle is off
    • schedule_series with pinned channel_id blocks disabled channels when toggle is off

Test updates:

  • Added feature coverage in BrowseShowsTest, GuestBrowseShowsTest, and DvrScheduleToolTest for include-disabled behavior.
  • Added test-environment broadcast override to prevent Reverb/Pusher network calls during feature tests.

Validation run:

  • vendor/bin/pint --format agent
  • php artisan test --compact tests/Feature/EpgApiControllerTest.php tests/Feature/EpgGroupsTest.php tests/Feature/DvrSeriesDefaultsTest.php tests/Feature/DvrRecorderServiceTest.php
  • php artisan test --compact tests/Feature/BrowseShowsTest.php tests/Feature/GuestBrowseShowsTest.php
  • php artisan test --compact tests/Feature/DvrScheduleToolTest.php

All above are passing locally after the changes.

Grimothy added 4 commits May 25, 2026 14:43
…channels table

Root cause: Channel::factory()->for(->playlist)->create() evaluates
the nested Playlist::factory() in the factory definition, creating an
extra Playlist record that triggers ProcessM3uImport via the
PlaylistCreated event. With QUEUE_CONNECTION=sync, this runs inline
during the test, interfering with test data.

Additionally, ChannelScrubber::create() triggers its creating event
(overwriting uuid with auto-generated value) and created event
(dispatching ProcessChannelScrubber which overwrites uuid again).
With sync queue, by the time the test reads ->scrubber->uuid,
the DB has a different value, causing the batch UUID guard in
ProcessChannelScrubberChunk::handle() to bail early.

Fix: Pass IDs directly (playlist_id, user_id, group_id) in the
create array to avoid nested factory evaluation. Use
ChannelScrubber::createQuietly() to prevent the created event
from dispatching ProcessChannelScrubber that overwrites the uuid.
PostgreSQL uuid column rejects the plain string 'test-batch-uuid'.
Use a properly formatted UUID to satisfy the column type constraint,
while keeping createQuietly() to prevent ProcessChannelScrubber
from overwriting the uuid.
…sTest

Same pattern as ProcessChannelScrubberChunkTest fix: pass foreign key
IDs directly instead of using ->for() to avoid nested Playlist::factory()
evaluation that creates extra Playlists and triggers ProcessM3uImport,
which leaks notifications and violates Notification::fake().
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.

3 participants