Skip to content

Storage: cross-record race between save_episode and delete_podcast #241

Description

@lqdev

Cross-record race: episode save can interleave with podcast delete

Context

Surfaced by Copilot review on PR #240 (#234, per-record locking):

Episode writes only acquire an episode:<podcast_id>:<episode_id> lock, while delete_podcast acquires podcast:<id> and removes the whole episode directory/cache entry. A concurrent podcast delete and episode save for the same podcast can still interleave so the delete removes the file or cache entry between this write and cache update, leaving orphan episode data or cache state that disagrees with disk.

This is a preexisting race (the same hole existed before #234) but #234 is the natural place to surface it because it locks per record but doesn't address cross-record dependencies.

Reproduction (theoretical)

  1. Task A: save_episode(p, e) — acquires episode:p:e, writes file, updates episode cache entry.
  2. Task B: delete_podcast(p) — acquires podcast:p, removes episode dir on disk, evicts all episodes from cache.
  3. If A's atomic_write + cache update straddles B's delete, you can end up with:
    • Disk: episode file gone (B deleted the dir), or
    • Cache: episode entry present (A re-inserted it after B's eviction).

The cache and disk now disagree until process restart.

The same shape also affects delete_podcast vs save_playlist if a playlist references the deleted podcast (cross-aggregate consistency rather than cross-record, but same flavor).

Approach options

  1. Hierarchical locking: save_episode/delete_episode also take the podcast:<podcast_id> lock (in a fixed order to prevent deadlock — podcast first, then episode). Simple, scales fine, but adds contention across all episodes of the same podcast.
  2. RwLock per podcast: episodes take a read guard on podcast:<id>, delete_podcast takes a write guard. Less contention for parallel episode writes; more invasive.
  3. Tombstone / generation number: each podcast gets a monotonic generation; save_episode records the gen it observed and refuses to commit if delete_podcast has bumped it. Lock-free but needs careful design.

Option 1 is the obvious starting point and matches the locking style already in use.

Acceptance criteria

  • Pick approach + document choice in code comment
  • Concurrent save_episode + delete_podcast for same podcast id leave cache and disk consistent (either episode is gone from both, or present in both with matching content)
  • No deadlock under concurrent save_episode for many podcasts in parallel (verify with stress test)
  • Same coverage for save_playlist if playlists hold strong references that delete_podcast could orphan
  • Regression test in src/storage/json.rs::tests
  • No regression in examples/bench_storage_load.rs

References

Metadata

Metadata

Assignees

No one assigned

    Labels

    P2Medium prioritybugSomething isn't workingstorageStorage / persistence component

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions