fix: stop treating HTTP 304 as a feed error (#9)

[dubadub]

Fixes #9.

What was broken

Two bugs compounded into a feed that dies permanently and silently.

A 304 was treated as a fetch failure. fetcher.rs turned HTTP 304 into Err(Error::FeedParse("304 Not Modified")) — smuggling a success through the error type, identified downstream by string matching. crawl_feed never did that string match, so a healthy 304 fell into its generic error handler and set status = "error".

Errored feeds were never retried. The scheduler only listed feeds with status = "active", making "error" a terminal state. Only a process restart unstuck a feed, because config sync resets the status on boot.

Together: every feed whose server correctly honours ETags gets bricked on the first crawl after a successful one. That is exactly the log in #9 — 13:38 success (stores ETag) → 14:38 conditional request → 304 → error → silence.

The smoking gun: Crawler::fetch_feed was the only function that handled 304 correctly, and it had zero callers. Dead code. The live path through crawl_feed had simply forgotten the case, and the dead duplicate made it look handled.

The fix

• fetch_with_conditions now returns FetchOutcome::{Fetched, NotModified} instead of Result<FetchResult>, so the compiler forces every caller to confront the 304 case. Adding a third string-match would have repeated the exact fragility that caused this.
• On 304 the crawler records a healthy no-op poll via mark_feed_unchanged, which clears stale error state but deliberately preserves etag/last_modified — clearing them would make the next request unconditional.
• The scheduler polls via list_crawlable_feeds, which includes error feeds so transient failures self-heal, while still excluding deliberately disabled ones.
• Deleted the dead fetch_feed/FetchedFeed pair.

One adjacent bug fixed because it lives in the same call: crawl_feed sent If-Modified-Since as RFC 3339, which is not a valid HTTP-date at all, and the recipe path used chrono's to_rfc2822, which renders the zone as +0000 instead of GMT. Servers were ignoring the header, so feeds without an ETag re-downloaded every hour. Both now use a proper IMF-fixdate helper.

Tests

Four mockito integration tests in tests/conditional_request_test.rs: 304-via-ETag, 304-via-Last-Modified, a normal 200 with caching headers, and confirmation that a 500 still errors so genuinely broken feeds are still flagged. Plus DB tests pinning the retry-errored-but-skip-disabled selection and the 304 bookkeeping.

SSRF validation blocks loopback URLs (validation.rs:70), so crawl_feed cannot be driven end-to-end against a mock server — I tested the two real seams instead rather than weaken that check for tests.

Full suite passes (80 tests), clippy and rustfmt clean.

Deployment

No migration needed. Feeds currently stuck in error are picked up automatically on the next scheduler tick.

[claude]

Review

Solid fix — good root-cause diagnosis (dead fetch_feed masking the missing 304 handling in the live crawl_feed path), and the fix itself removes the fragility class it's curing rather than patching around it.

Strengths

• FetchOutcome::{Fetched, NotModified} instead of string-matching an Error::FeedParse("304 Not Modified"). This is the right fix for the actual root cause: it makes 304-handling a compiler-checked exhaustive match instead of a string comparison that's one refactor away from silently breaking again (which is exactly what happened here).
• mark_feed_unchanged correctly preserves etag/last_modified while clearing error state — verified by mark_feed_unchanged_clears_error_state_and_keeps_validators. Clearing the validators on a 304 would have been an easy, subtle mistake (next request becomes unconditional, full body re-downloaded).
• list_crawlable_feeds scoping is correct: error feeds are retried, disabled feeds (deliberately turned off via config) stay excluded, and it's a separate function rather than overloading list_feeds, so the admin-facing status filter (list_feeds_with_filter) is untouched. The existing idx_feeds_status index covers the IN ('active','error') predicate fine.
• http_date fix is a real second bug, not scope creep — RFC 3339 is flatly invalid as an If-Modified-Since value, and to_rfc2822()'s +0000 vs GMT would have made every feed without an ETag re-download hourly. Good catch, and the dedicated http_date_renders_imf_fixdate test pins the exact case that broke before.
• Dead fetch_feed/FetchedFeed removed cleanly; confirmed no other callers remain in the codebase.
• Tests target real seams (304-via-ETag, 304-via-Last-Modified, 200 with caching headers, 500-still-errors) rather than over-mocking, and the retryable-vs-not distinction is preserved (500 → Error::FeedParse, which isn't retried by is_retryable, so the 500 test stays fast rather than waiting through backoff).
• The SSRF note in the PR description (why crawl_feed itself isn't driven end-to-end against mockito) is the right call — weakening validation.rs's loopback check for test convenience would be a real regression.

Things worth a look (non-blocking)

1. Errored feeds now retry every scheduler tick forever, with no backoff or auto-disable ceiling. That's strictly correct for the bug being fixed (transient failures must not be terminal), but a permanently broken feed (dead domain, deleted account, a server that always answers with a bogus ETag) will now be re-fetched on every tick indefinitely — there's no backoff proportional to error_count, and the only way to stop it is a manual disabled via config sync. Not a bug, but might be worth a follow-up (skip a tick after N consecutive errors, or backoff based on error_count) so one dead feed doesn't eat crawl/rate-limit budget forever.
2. Pre-existing (untouched by this diff) but adjacent: crawl_feed's error branches call increment_error_count and then immediately update_feed_status(..., feed.error_count + 1, ...), which overwrites whatever increment_error_count just wrote with the same computed value from the pre-crawl in-memory feed. Harmless (same end value) but it's a redundant write on every single error path — worth folding into one call at some point, unrelated to this PR though.
3. Fetcher::fetch() (the new unconditional wrapper) has no direct test for its "304 to an unconditional request is an error" branch, and it looks unused elsewhere in src/ (only fetch_with_conditions has real callers). Not a problem, but if it's staying as public API rather than being pruned, a quick unit test would close the loop the way http_date's did.

Test coverage

Good balance of true integration tests (mockito HTTP semantics) and DB-level tests (retry-inclusion, unchanged-status bookkeeping) targeting exactly the two bugs from #9. The explicit "500 still errors" test is a nice touch — it guards against a fix that's too aggressive as well as one that's too narrow.

Nothing here blocks merging — the flagged items are forward-looking, not defects in this diff.