Skip to content

No test covers the FetchResult::NotModified (304) path or store_validators in the feed scheduler #456

Description

@forkwright

Finding

The conditional-GET machinery in FeedSchedulerService is untested end to end. The FetchResult::NotModified arms of refresh_podcast_feed and refresh_news_feed (which skip parsing and return new_items: 0) are never exercised, and the cache_validators in-memory map populated by store_validators/read by cached_validators is never asserted on: whether validators are stored, retrieved, and forwarded as If-None-Match / If-Modified-Since on the next refresh is untested.

Evidence

crates/komide/src/service/mod.rs:321 — the podcast 304 arm (mirrored for news at lines 381-391):

FetchResult::NotModified => {
    debug!(feed_id = %feed_id, "podcast feed not modified (304)");
    let total =
        podcast::count_episodes_for_subscription(&self.db.read, sub.id.as_slice())
            .await
            .context(DatabaseSnafu)? as usize;
    Ok(FeedRefreshResult {
        feed_id,
        new_items: 0,
        total_items: total,
    })
}

crates/komide/src/service/mod.rs:535-550 — the untested validator cache, including the store guard:

async fn cached_validators(&self, url: &str) -> (Option<String>, Option<String>) {
    let cache = self.cache_validators.lock().await;
    cache.get(url).cloned().unwrap_or((None, None))
}

async fn store_validators(
    &self,
    url: &str,
    etag: Option<String>,
    last_modified: Option<String>,
) {
    if etag.is_some() || last_modified.is_some() {
        let mut cache = self.cache_validators.lock().await;
        cache.insert(url.to_string(), (etag, last_modified));
    }
}

Searching service/tests.rs for NotModified, 304, cached_validators, and store_validators returns no matches.

Why this matters

The 304 path bypasses parsing and recomputes total_items from a separate DB query; a regression in that query (wrong subscription/feed scoping, off-by-one) would silently misreport feed state to the UI without any parse step to cross-check it. The store_validators guard (etag.is_some() || last_modified.is_some()) is the on-switch for conditional GET — if it regressed so validators were never stored, every refresh would re-download the full feed body, defeating the bandwidth saving. On a sovereign phone that is exactly the cost that matters: needless full-feed fetches inflate metered/observable traffic and battery use, and an adversary watching network volume sees a louder, more regular signature. No test currently guards either behaviour.

Desired correction

Add integration tests using an in-process mock fetch_feed shim (or dependency injection on the HTTP client) covering: (1) a 304 response on a known feed returns new_items: 0 with the correct total_items from the DB; (2) after a 200 carrying an ETag, the next refresh forwards it as If-None-Match (assert via the mock that the conditional header is sent); (3) store_validators called with both values None does NOT insert into the cache (the guard holds). Cover at least one of the podcast and news paths, given their structures are mirrored.
Done when: cargo test -p komide includes at least two tests that exercise FetchResult::NotModified through the service layer and assert the validator cache is populated and forwarded.

Metadata

Metadata

Assignees

No one assigned

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions