From 0a8a3451c4f29ae20ef00f222b34ed1fdd6506f5 Mon Sep 17 00:00:00 2001 From: hl-valdemar Date: Mon, 26 Oct 2026 15:29:17 +0100 Subject: [PATCH] fix: close playback progress readers between polls --- README.md | 2 ++ src/video_state.rs | 17 ++++++----- tests/unit/video_state.rs | 64 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 75 insertions(+), 8 deletions(-) diff --git a/README.md b/README.md index 231f90c..0ab9587 100644 --- a/README.md +++ b/README.md @@ -363,6 +363,8 @@ State writes are atomic and owner-only. Invalid state files or detected external Progress sessions survive Tubular exit so later player reports can be recovered on the next launch. Session directories are `0700`; snapshots, identity markers and Terebi-created reports are owner-only. New Miruro launches contain no resolved-source documents or CDN credentials. Existing sessions from earlier versions remain readable and are not automatically removed. Sessions are retained because CLI exit does not prove the independent GUI has finished; up to 4,096 are monitored. Close the corresponding players before removing their inactive session directories, then restart Tubular. Never remove an active player's directory. +Progress polling keeps byte offsets and partial records in memory, opening and closing each report for each poll. Historical sessions do not retain open report descriptors; late writes from running players are still consumed. + ## Saved videos Saved videos is a separate stored collection from watch later, deduplicated by YouTube video ID or stable Miruro episode identity, with provider-specific views. It uses offline saved metadata; YouTube entries follow **current channel category memberships**, not categories stored on individual videos. Its All categories (combined) list contains only YouTube videos; Miruro opens a flat saved-episode list. diff --git a/src/video_state.rs b/src/video_state.rs index 5837d34..28bbde0 100644 --- a/src/video_state.rs +++ b/src/video_state.rs @@ -1,7 +1,7 @@ use std::{ collections::BTreeSet, fs::{self, File, OpenOptions, ReadDir}, - io::{self, Read, Write}, + io::{self, Read, Seek, SeekFrom, Write}, path::{Path, PathBuf}, sync::atomic::{AtomicU64, Ordering}, }; @@ -1051,7 +1051,7 @@ struct ProgressStream { seen: bool, status_edited: bool, started: bool, - file: Option, + offset: u64, partial: Vec, discarding: bool, failed: bool, @@ -1068,7 +1068,7 @@ impl ProgressStream { seen: false, status_edited: false, started: false, - file: None, + offset: 0, partial: Vec::new(), discarding: false, failed: false, @@ -1184,12 +1184,13 @@ impl ProgressStream { pending: &mut BTreeSet, watched: &BTreeSet, ) -> io::Result> { - if self.file.is_none() { - self.prepare()?; - self.file = Some(regular_file(&self.path, true)?); - } + self.prepare()?; + // Keep a cursor, not a descriptor, for each retained playback session. + let mut file = regular_file(&self.path, true)?; + file.seek(SeekFrom::Start(self.offset))?; let mut buffer = [0; STREAM_BYTES]; - let size = self.file.as_mut().unwrap().read(&mut buffer)?; + let size = file.read(&mut buffer)?; + self.offset += size as u64; let mut error = None; for &byte in &buffer[..size] { if byte == b'\n' { diff --git a/tests/unit/video_state.rs b/tests/unit/video_state.rs index a77c712..6f64e38 100644 --- a/tests/unit/video_state.rs +++ b/tests/unit/video_state.rs @@ -580,6 +580,70 @@ fn watched_write_failure_keeps_memory_unchanged_and_retries_consumed_reports() { assert!(!state.poll_progress().unwrap()); } +#[cfg(unix)] +#[test] +fn historical_progress_sessions_leave_descriptors_available_for_subprocesses() { + const CHILD: &str = "TUBULAR_PROGRESS_DESCRIPTOR_TEST"; + if std::env::var_os(CHILD).is_none() { + let output = std::process::Command::new(std::env::current_exe().unwrap()) + .args([ + "--exact", + "video_state::tests::historical_progress_sessions_leave_descriptors_available_for_subprocesses", + "--test-threads=1", + ]) + .env(CHILD, "1") + .output() + .unwrap(); + assert!( + output.status.success(), + "{}\n{}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); + return; + } + + // Isolate the process-wide limit from the rest of the test suite. + let mut limit = libc::rlimit { + rlim_cur: 0, + rlim_max: 0, + }; + // SAFETY: limit is valid writable storage; only this child process is affected. + unsafe { + assert_eq!(libc::getrlimit(libc::RLIMIT_NOFILE, &mut limit), 0); + limit.rlim_cur = limit.rlim_cur.min(64); + assert_eq!(libc::setrlimit(libc::RLIMIT_NOFILE, &limit), 0); + } + + let directory = TestDirectory::new(); + let mut state = State::open(&directory.config()).unwrap(); + let video = video(); + let reports = state.progress_files(&vec![video.clone(); 96]).unwrap(); + for report in &reports { + register_identity(report, &video.id).unwrap(); + append(report, record(&video.id, "progress", false, 0.0).as_bytes()); + } + let last = reports.last().unwrap(); + let completion = record(&video.id, "closed", true, 95.0); + append(last, &completion.as_bytes()[..completion.len() - 1]); + drop(state); + + let mut state = State::open(&directory.config()).unwrap(); + for _ in 0..12 { + state.poll_progress().unwrap(); + } + assert!(!state.is_watched(&video.id)); + append(last, b"\n"); + for _ in 0..6 { + state.poll_progress().unwrap(); + } + assert!(state.is_watched(&video.id)); + let output = std::process::Command::new("/usr/bin/true") + .output() + .expect("progress monitoring must leave descriptors available for subprocess pipes"); + assert!(output.status.success()); +} + #[test] fn partial_reports_survive_restart_and_bad_siblings_do_not_block_completion() { let directory = TestDirectory::new();