Skip to content

fix: bound offline media storage and add regression coverage - #19

Merged
sidinsearch merged 1 commit into
sidinsearch:experimental/video-playbackfrom
DriftyPencil:experimental/video-playback
Sep 7, 2026
Merged

sidinsearch merged 1 commit into
sidinsearch:experimental/video-playbackfrom
DriftyPencil:experimental/video-playback

Conversation

@DriftyPencil

Copy link
Copy Markdown

Summary

Saved Instagram reels and YouTube videos can be downloaded to the device and played in a vertical feed. The backend serves MP4s through an authenticated endpoint, while the mobile app verifies local files, falls back to streaming, and repairs missing device copies in the background.

Server storage policy

The server media cache now:

  • Expires MP4s 30 days after successful publication by default.
  • Runs cleanup at startup and every hour.
  • Removes unreferenced and soft-deleted media after a one-hour grace period.
  • Removes abandoned download workspaces and legacy download fragments.
  • Publishes completed files atomically.
  • Coordinates publication and cleanup across API workers.
  • Updates database metadata when cached media is removed.

Downloaded device copies remain usable after server-cache expiry.

Tests

Added coverage for:

  • Media endpoint authentication.
  • Invalid and traversal filenames.
  • Missing files and directories.
  • Valid MP4 responses and video/mp4 content type.
  • Retention boundaries and orphan cleanup.
  • Failed deletion and database failure handling.
  • Active download protection and cross-process locking.
  • Local and remote video source selection.
  • Missing-file recovery and re-download behavior.
  • OfflineMediaManager, ReelsFeed, StorageManager, and local database behavior.

The focused CI workflow runs backend tests, Jest/RNTL tests, and TypeScript checks.

Storage planning

At approximately 50 MB per video:

  • 100 retained videos use about 5 GB.
  • 1,000 retained videos use about 50 GB.
  • 10 videos per day with 30-day retention use about 15 GB.
  • 100 videos per day with 30-day retention use about 150 GB.

Retention limits file age, not total disk usage. Operators requiring an absolute disk limit should configure a filesystem or Docker volume quota.

yt-dlp remains in the backend image. Splitting downloads into a separate worker is a possible follow-up.

This is a follow-up to #13. Proposed promotion path: experimental/video-playbackbetamain, pending maintainer review.

@sidinsearch
sidinsearch merged commit 9b06f93 into sidinsearch:experimental/video-playback Sep 7, 2026
@sidinsearch

sidinsearch commented Sep 7, 2026

Copy link
Copy Markdown
Owner

Merged into experimental/video-playback

This is genuinely excellent work. Merging now. A few notes on the configurability question (and a couple of other things I noticed) below.


Is the policy user-configurable?

✅ Time (TTL) — yes:

  • MEDIA_RETENTION_DAYS (default: 30, must be positive int)
  • MEDIA_CLEANUP_INTERVAL_SECONDS (default: 3600)
  • Invalid/zero/negative values fall back to defaults with a warning (positive_env_int)

❌ Total disk usage — no:

This is the big one. The policy bounds age, not bytes. Your own docs/OFFLINE_MEDIA.md says it honestly:

"Retention limits age, not total disk usage. A burst of downloads can still fill the disk before expiry. ... Use a filesystem/volume quota when an absolute disk cap is required."

That's a real gap. If a deployment has a 100 GB volume, a 14-day burst of 100 videos/day × 50 MB × 14 days = 70 GB of MP4s that haven't expired yet — and MEDIA_RETENTION_DAYS doesn't help.

Suggested follow-up (small PR):

# in media_retention.py
MAX_BYTES = positive_env_int("MEDIA_MAX_BYTES", 0)  # 0 = disabled

# inside sweep_media, after the expired/orphan pass:
if MAX_BYTES > 0:
    while total_bytes() > MAX_BYTES:
        oldest = next_oldest_active()
        if not oldest:
            break
        evict(oldest)

Plus MEDIA_MAX_BYTES=0 should disable it. Three-line change, ships a real operator-facing feature.


Other things I noticed (none are blockers — just observations)

1. No observability for the cache

There's no admin endpoint to query current cache state — operators can only see the logger.info("Media cache sweep: %s", result) line in stdout. For prod deployments, consider exposing:

  • GET /admin/media-cache/stats{file_count, total_bytes, oldest_age_days, newest_age_days, last_sweep_at} (auth-protected)
  • POST /admin/media-cache/sweep → run sweep on demand (auth-protected)

This pairs naturally with the StorageManager.tsx UI on the client.

2. MEDIA_PATH is referenced but not declared in media_retention.py

media_store.py reads MEDIA_PATH from env. The new media_retention.sweep_media() takes media_dir as an arg (good!), but the lifespan handler in api.py calls get_media_dir() — that's consistent, but worth a sanity check that the two paths are guaranteed to match.

3. Failed-deletion retry policy is implicit

If path.unlink() keeps failing (permissions, file locked on Windows), the file stays put and updated_at isn't bumped. Next sweep, the file is still old enough to retry — so it's fine. But there's no exponential backoff or operator alert if a single file fails 100 sweeps in a row. Probably fine for now.

4. pytest tests/test_connect_info_security.py still passes ✅ — verified when integrating this branch's parent.

5. offline-media-tests.yml workflow trigger is correct

It triggers on PR + push to experimental/video-playback + manual dispatch. After this merges, the workflow will start running on every push to the experimental branch — good.


What's great (the bulk of the review)

  • SQLite-IMMEDIATE cross-process locksmedia_lock() is the right primitive. Works on Linux/Windows/macOS without external deps.
  • Atomic publication via tempfile.TemporaryDirectory + Path.replace — no partial writes ever visible.
  • Path validation hardened with explicit /, \, \x00 checks on top of Path().name != filename.
  • Grace period of 1 hour for newly published files — protects the gap between download finishing and analysis row being saved.
  • Failed deletions are logged + retried — no silent data loss.
  • Test coverage is real: 417 lines on retention boundaries, 196 on the endpoint, 551 on the client. Covers auth, traversal, missing files, content-type, retention edges, locking semantics, source switching, recovery flows.
  • docs/OFFLINE_MEDIA.md is honest about what the policy doesn't solve (total disk cap).
  • CI workflow offline-media-tests.yml runs backend tests + Jest + typecheck.

This unblocks moving from experimental/video-playbackbeta for integration testing.


Suggested next steps (in priority order)

  1. Open a small follow-up PR adding MEDIA_MAX_BYTES for total-size capping (the gap I flagged at the top).
  2. Add /admin/media-cache/stats endpoint so operators can monitor cache health.

Great work. This is what a thoughtful PR looks like.

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