feat(server): add periodic podcast synchronization#720
Conversation
hasezoey
left a comment
hasezoey
left a comment
There was a problem hiding this comment.
Thanks for trying your hand at this feature, but i think the current design is not quite right.
In my opinion the podcasts themself should store a last_checked (or next_check_at) date and potentially a check_interval.
This information should then be used to fetch to podcast changes.
Additionally, i dont like that the new episodes get automatically added to the playlist, as currently they can (theoretically) appear in any order between any podcast that gets synchronized and if there is a episode that gets skipped (ex. network error) other episodes just still get appended and that there is currently no way to turn this off.
Finally, i think before this feature is added, the whole existing podcast sync should be moved to the server instead of being handled by the TUI.
Side notes:
- the commit message style, specifically the scope, is not quite what we use here (see recent commits and CONTRIBUTING as reference)
- there should only be one global TaskPool for network (or at the very least podcast) tasks
- things i likely forgot already trying to review the massive changes in the past 3 hours
| /// Podcast synchronization settings (automatic feed refresh and download). | ||
| pub synchronization: SynchronizationSettings, |
There was a problem hiding this comment.
If that is podcast related, it should be under podcast
| /// Whether automatic podcast synchronization is enabled. | ||
| /// Default: true | ||
| pub enable: bool, | ||
|
|
||
| /// How often to check all subscribed feeds for new episodes. | ||
| /// Accepts human-readable duration strings: "1h", "30m", "2h30m". | ||
| /// Default: "1h" (3600 seconds) | ||
| #[serde(with = "humantime_serde")] | ||
| pub interval: Duration, |
There was a problem hiding this comment.
These 2 options could be condensed into one where interval is set to 0 to disable it
| /// Maximum number of new episodes to download per podcast per sync pass. | ||
| /// Only the newest N undownloaded episodes are fetched. | ||
| /// Set to 0 for unlimited (download all missing episodes). | ||
| /// Default: 5 | ||
| pub max_new_episodes: u32, |
There was a problem hiding this comment.
This should be able to be disabled. Consider either using a enum.
| fn default() -> Self { | ||
| Self { | ||
| enable: true, | ||
| interval: Duration::from_secs(3600), |
There was a problem hiding this comment.
A quick comment as to how much time that is in human readable would be good here too (i know it is in the option above too).
| /// Inner helper for raw deserialization without the wrapping logic. | ||
| #[derive(Deserialize)] | ||
| #[serde(default)] | ||
| struct SyncSettingsRaw { | ||
| enable: bool, | ||
| #[serde(with = "humantime_serde")] | ||
| interval: Duration, | ||
| refresh_on_startup: bool, | ||
| max_new_episodes: u32, | ||
| /// When the struct is deserialized from a standalone TOML document that | ||
| /// contains `[synchronization]` as a section header, this field captures | ||
| /// the nested table so we can unwrap it. | ||
| synchronization: Option<SyncSettingsNested>, | ||
| } | ||
|
|
||
| impl Default for SyncSettingsRaw { | ||
| fn default() -> Self { | ||
| let defaults = SynchronizationSettings::default(); | ||
| Self { | ||
| enable: defaults.enable, | ||
| interval: defaults.interval, | ||
| refresh_on_startup: defaults.refresh_on_startup, | ||
| max_new_episodes: defaults.max_new_episodes, | ||
| synchronization: None, | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /// The nested representation when parsing a standalone TOML with `[synchronization]` header. | ||
| #[derive(Deserialize)] | ||
| struct SyncSettingsNested { | ||
| #[serde(default = "default_enable")] | ||
| enable: bool, | ||
| #[serde(default = "default_interval", with = "humantime_serde")] | ||
| interval: Duration, | ||
| #[serde(default = "default_refresh_on_startup")] | ||
| refresh_on_startup: bool, | ||
| #[serde(default = "default_max_new_episodes")] | ||
| max_new_episodes: u32, | ||
| } | ||
|
|
||
| fn default_enable() -> bool { | ||
| true | ||
| } | ||
|
|
||
| fn default_interval() -> Duration { | ||
| Duration::from_secs(3600) | ||
| } | ||
|
|
||
| fn default_refresh_on_startup() -> bool { | ||
| true | ||
| } | ||
|
|
||
| fn default_max_new_episodes() -> u32 { | ||
| 5 | ||
| } | ||
|
|
||
| /// Custom Deserialize implementation that supports dual-path deserialization: | ||
| /// | ||
| /// 1. **Nested path**: When parsed as a standalone TOML document containing a | ||
| /// `[synchronization]` section header (e.g., during config file testing or | ||
| /// isolated deserialization), the struct unwraps the nested table. | ||
| /// 2. **Flat path**: When parsed as a value within the parent `ServerSettings` | ||
| /// struct (the normal runtime case), fields are read directly. | ||
| /// | ||
| /// This dual-path approach is necessary because other config sections use | ||
| /// `#[serde(default)]` on derived `Deserialize`, but `SynchronizationSettings` | ||
| /// needs to handle both contexts without requiring callers to strip the section | ||
| /// header. The test suite validates both paths. | ||
| impl<'de> Deserialize<'de> for SynchronizationSettings { | ||
| fn deserialize<D>(deserializer: D) -> Result<Self, D::Error> | ||
| where | ||
| D: Deserializer<'de>, | ||
| { | ||
| let raw = SyncSettingsRaw::deserialize(deserializer)?; | ||
|
|
||
| // If the TOML had a [synchronization] section at the root level, | ||
| // the nested struct will be populated. Use its values instead. | ||
| if let Some(nested) = raw.synchronization { | ||
| Ok(Self { | ||
| enable: nested.enable, | ||
| interval: nested.interval, | ||
| refresh_on_startup: nested.refresh_on_startup, | ||
| max_new_episodes: nested.max_new_episodes, | ||
| }) | ||
| } else { | ||
| Ok(Self { | ||
| enable: raw.enable, | ||
| interval: raw.interval, | ||
| refresh_on_startup: raw.refresh_on_startup, | ||
| max_new_episodes: raw.max_new_episodes, | ||
| }) | ||
| } | ||
| } | ||
| } |
| assert_eq!( | ||
| stats2.episodes_downloaded, 0, | ||
| "second pass should not re-download" | ||
| ); | ||
| assert_eq!( | ||
| stats2.episodes_enqueued, 0, | ||
| "second pass should not re-enqueue" | ||
| ); |
There was a problem hiding this comment.
This should also have a test that 1 podcast has been checked
| PlayerCmd::PlaylistAddTrack(add_track) => { | ||
| assert_eq!(add_track.at_index, PlaylistAddTrack::AT_END); | ||
| assert_eq!(add_track.tracks.len(), 1); |
There was a problem hiding this comment.
It should also test the correct episode is downloaded
| /// SCENARIO-016: When a new episode is enqueued via PlaylistAddTrack, the | ||
| /// command uses the correct format that triggers auto-start when the queue | ||
| /// was empty (existing player loop behavior). | ||
| #[tokio::test] | ||
| async fn integration_enqueue_format_enables_autostart_on_empty_queue() { | ||
| let mock_server = MockServer::start().await; | ||
|
|
||
| let ep_path = "/episodes/autostart.mp3"; | ||
| let feed_xml = generate_rss_feed( | ||
| "Autostart Test", | ||
| &[( | ||
| "Fresh Episode", | ||
| "guid-autostart-001", | ||
| &format!("{}{}", mock_server.uri(), ep_path), | ||
| )], | ||
| ); | ||
|
|
||
| Mock::given(method("GET")) | ||
| .and(path("/autostart_feed.xml")) | ||
| .respond_with( | ||
| ResponseTemplate::new(200) | ||
| .set_body_string(feed_xml) | ||
| .insert_header("content-type", "application/rss+xml"), | ||
| ) | ||
| .mount(&mock_server) | ||
| .await; | ||
|
|
||
| Mock::given(method("GET")) | ||
| .and(path(ep_path)) | ||
| .respond_with( | ||
| ResponseTemplate::new(200) | ||
| .set_body_bytes(fake_audio_content()) | ||
| .insert_header("content-type", "audio/mpeg"), | ||
| ) | ||
| .mount(&mock_server) | ||
| .await; | ||
|
|
||
| let tmp_dir = tempfile::tempdir().expect("create temp dir"); | ||
| let db_path = tmp_dir.path(); | ||
| let download_dir = tmp_dir.path().join("downloads"); | ||
| std::fs::create_dir_all(&download_dir).expect("create download dir"); | ||
|
|
||
| let db = Database::new(db_path).expect("create database"); | ||
| let podcast = PodcastNoId { | ||
| title: "Autostart Test".to_string(), | ||
| url: format!("{}/autostart_feed.xml", mock_server.uri()), | ||
| description: None, | ||
| author: None, | ||
| explicit: None, | ||
| last_checked: chrono::Utc::now(), | ||
| episodes: vec![], | ||
| image_url: None, | ||
| }; | ||
| db.insert_podcast(&podcast).expect("insert podcast"); | ||
| drop(db); | ||
|
|
||
| let config = make_test_config(&download_dir); | ||
| let (cmd_tx, mut cmd_rx) = make_cmd_channel(); | ||
|
|
||
| let result = sync_once(&config, &cmd_tx, db_path).await; | ||
| assert!(result.is_ok()); | ||
|
|
||
| let (cmd, _) = cmd_rx.try_recv().expect("should receive command"); | ||
| match cmd { | ||
| PlayerCmd::PlaylistAddTrack(add_track) => { | ||
| assert_eq!( | ||
| add_track.at_index, | ||
| PlaylistAddTrack::AT_END, | ||
| "must use AT_END for auto-start detection" | ||
| ); | ||
| assert!( | ||
| !add_track.tracks.is_empty(), | ||
| "must have at least one track to trigger playback" | ||
| ); | ||
| } | ||
| other => panic!("Expected PlaylistAddTrack, got: {:?}", other), | ||
| } | ||
| } |
There was a problem hiding this comment.
This does not actually test anything new and especially not that the player actually starts
| // ========================================================================= | ||
| // T-26: Task lifecycle integration tests | ||
| // SCENARIO-022: Sync pass during ongoing playback does not disrupt audio | ||
| // ========================================================================= | ||
|
|
||
| /// During active playback (simulated), a sync pass should append new episodes | ||
| /// at the END without interrupting the current track. | ||
| /// | ||
| /// AC-07, SCENARIO-022: Sync during playback does not disrupt audio. | ||
| #[tokio::test] | ||
| async fn integration_sync_during_playback_appends_at_end() { | ||
| let mock_server = MockServer::start().await; | ||
|
|
||
| let ep_path = "/episodes/during_playback.mp3"; | ||
| let feed_xml = generate_rss_feed( | ||
| "Playback Test Podcast", | ||
| &[( | ||
| "New During Playback", | ||
| "guid-playback-001", | ||
| &format!("{}{}", mock_server.uri(), ep_path), | ||
| )], | ||
| ); | ||
|
|
||
| Mock::given(method("GET")) | ||
| .and(path("/playback_feed.xml")) | ||
| .respond_with( | ||
| ResponseTemplate::new(200) | ||
| .set_body_string(feed_xml) | ||
| .insert_header("content-type", "application/rss+xml"), | ||
| ) | ||
| .mount(&mock_server) | ||
| .await; | ||
|
|
||
| Mock::given(method("GET")) | ||
| .and(path(ep_path)) | ||
| .respond_with( | ||
| ResponseTemplate::new(200) | ||
| .set_body_bytes(fake_audio_content()) | ||
| .insert_header("content-type", "audio/mpeg"), | ||
| ) | ||
| .mount(&mock_server) | ||
| .await; | ||
|
|
||
| let tmp_dir = tempfile::tempdir().expect("create temp dir"); | ||
| let db_path = tmp_dir.path(); | ||
| let download_dir = tmp_dir.path().join("downloads"); | ||
| std::fs::create_dir_all(&download_dir).expect("create download dir"); | ||
|
|
||
| let db = Database::new(db_path).expect("create database"); | ||
| let podcast = PodcastNoId { | ||
| title: "Playback Test Podcast".to_string(), | ||
| url: format!("{}/playback_feed.xml", mock_server.uri()), | ||
| description: None, | ||
| author: None, | ||
| explicit: None, | ||
| last_checked: chrono::Utc::now(), | ||
| episodes: vec![], | ||
| image_url: None, | ||
| }; | ||
| db.insert_podcast(&podcast).expect("insert podcast"); | ||
| drop(db); | ||
|
|
||
| let config = make_test_config(&download_dir); | ||
| let (cmd_tx, mut cmd_rx) = make_cmd_channel(); | ||
|
|
||
| let result = sync_once(&config, &cmd_tx, db_path).await; | ||
| assert!(result.is_ok()); | ||
|
|
||
| let (cmd, _) = cmd_rx.try_recv().expect("should receive command"); | ||
| match cmd { | ||
| PlayerCmd::PlaylistAddTrack(add_track) => { | ||
| assert_eq!( | ||
| add_track.at_index, | ||
| PlaylistAddTrack::AT_END, | ||
| "sync during playback must append at end" | ||
| ); | ||
| } | ||
| other => panic!("Expected PlaylistAddTrack, got: {:?}", other), | ||
| } | ||
| } |
There was a problem hiding this comment.
This does not test anything new
| .mount(&mock_server) | ||
| .await; | ||
|
|
||
| // Do NOT mock the episode download -- it should never be requested |
There was a problem hiding this comment.
It should likely exist, but use a spy to confirm it was never hit
|
@hasezoey since in your review, i saw there a huge redesign decision, move the sync from tui to server, can you confirm if we can move on or wait for your decision ? |
It was not just that decision, it was also that i would recommend / prefer if we go with a "last_checked" / "next_check_at" in database instead of always directly re-syncing approach. @tramhao what do you think which approach would be better? |
Critical fixes (P0): - Use PlaylistTrackSource::PodcastUrl instead of Path at both enqueue points Concurrency/async fixes (P1): - Replace blocking std::fs::read_dir with tokio::task::spawn_blocking - Consolidate per-podcast TaskPool into single shared instance - Restructure into collect-then-download pattern (feeds before downloads) Config simplification (P2): - Simplify SynchronizationSettings deserialization - Add auto_enqueue field with default true - Extract ensure_podcast_dir utility to lib/src/utils.rs - Filter played episodes from download consideration - Extract process_feed_result and enqueue_episode helpers Style/polish (P3): - Convert to //! module doc comments - Derive Default for SyncPassStats and use it - Use indoc! for test TOML strings - Assert specific error messages in tests - Delegate PlaylistAddTrack append methods to positional constructors Test coverage: 275 tests across lib + server, zero clippy warnings.
7029a9b to
8b79fe3
Compare
|
I fixed the bugs, but still left 2 strategic design questions that fundamentally change the architecture
|
|
I gave the changes a skim, but it is very hard to read due to all the noise and changes being set as "unviewed" even though nothing had changed (due to it being a single force-pushed commit). Also, not many of my review comments (which are unrelated to the design decision) have seemingly not been addressed at all. PS: i saw that |
|
I appoligize for this, since I am not a rust developer, I implemented these changes via AI tool, my purpose is to modify the code to fit my own needs that I want to synchronize it in background. Really appreciated this project, since I used it for a long time. |
… fixes from the PR tramhao#720 review feedback. Module-level //! doc comments were added to podcast_sync.rs. The 5-tuple config destructuring anti-pattern was replaced with cloned struct references (sync_settings, podcast_settings). Deeply nested inline logic was extracted into four module-level helpers (drain_download_results, enqueue_downloaded_episodes, prepare_download_plan) with promoted module-level structs (EnqueueEntry, DownloadPlan). A 10-test style enforcement suite validates AC-29 (doc comments), AC-30 (nesting limits), and AC-31 (config struct references). Both tasks T-46 and T-47 are complete.
…ith tokio::fs note Add collected review feedback from PR tramhao#720 including inline comments, action items, and recommended next steps. Includes additional comment about using tokio::fs::read_dir instead of spawn_blocking wrapper.
… remediation Implements all review feedback from PR tramhao#720: - Per-podcast last_checked/check_interval scheduling (DB migration v2) - Config moved under [podcast.synchronization] - TUI→server migration: all podcast network calls routed through gRPC - Single shared TaskPool with concurrent_downloads_max - Auto-enqueue configurable (AutoEnqueue enum) - Filename detection fixed with sanitize + substring matching - tokio::fs::read_dir replaces spawn_blocking wrapper - Test quality: TestHarness, localhost URLs, specific assertions - Style: doc comments, helper extraction, config struct references
|
To prevent accidental merging, i have converted this to a DRAFT |
2 participants
Add server-internal scheduled job that periodically refreshes subscribed podcasts, downloads new episodes (deduplicated by GUID/enclosure URL), and appends them to the play queue via PlaylistAddTrack.
Features:
Tests: 79 total (unit + integration with wiremock mock HTTP server)