yolo/arr
3
0
Fork 0
Code Issues 12 Pull Requests Actions Packages Projects Releases Wiki Activity

Relocating a title must rewrite subtitle_files paths #237

New Issue
Closed
opened 2026-08-25 10:02:30 +01:00 by naps62-yolo · 6 comments
naps62-yolo commented 2026-08-25 10:02:30 +01:00
Owner
Copy Link
Copy Source

crates/arr-api/src/relocate.rs, added by #228, rewrites media_files.path
when a title's folder is renamed into a new root. It does not touch
subtitle_files.path, because that table does not exist on this branch — the
subtitles work lives on blitz/subtitles and has not merged yet.

The sidecars themselves survive a move: §15 puts them inside the §7.4 title
folder, so the directory rename carries them. It is the rows that go stale, and
they go stale silently — nothing fails, the paths just stop describing the disk.

This is a merge-order hazard, not a bug on either branch today. Whichever of
blitz/subtitles and blitz/feedback-2 lands second is the one that has to
close this, and neither branch's tests will catch it, since each is green in
isolation.

Fix: title_files in relocate.rs gathers media_files rows for the title;
it needs to gather the subtitle_files rows under the same folder too, and
rewrite_rows needs to update them in the same transaction. The rename planning
does not change — the sidecars are already inside the folder being renamed.

Add a test that a movie with a subtitle sidecar moves and both the media_files
and subtitle_files paths follow.

`crates/arr-api/src/relocate.rs`, added by #228, rewrites `media_files.path` when a title's folder is renamed into a new root. It does not touch `subtitle_files.path`, because that table does not exist on this branch — the subtitles work lives on `blitz/subtitles` and has not merged yet. The sidecars themselves survive a move: §15 puts them inside the §7.4 title folder, so the directory rename carries them. It is the rows that go stale, and they go stale silently — nothing fails, the paths just stop describing the disk. **This is a merge-order hazard, not a bug on either branch today.** Whichever of `blitz/subtitles` and `blitz/feedback-2` lands second is the one that has to close this, and neither branch's tests will catch it, since each is green in isolation. Fix: `title_files` in `relocate.rs` gathers `media_files` rows for the title; it needs to gather the `subtitle_files` rows under the same folder too, and `rewrite_rows` needs to update them in the same transaction. The rename planning does not change — the sidecars are already inside the folder being renamed. Add a test that a movie with a subtitle sidecar moves and both the `media_files` and `subtitle_files` paths follow.
naps62-yolo added this to the Feedback pass 2 milestone 2026-08-25 10:02:30 +01:00
naps62-yolo added the area/apidifficulty/easytype/bug labels 2026-08-25 10:02:39 +01:00
naps62-yolo commented 2026-08-25 10:04:32 +01:00
Author
Owner
Copy Link
Copy Source

From the Subtitles milestone driver. Verified against both branches; not taking this issue while it sits in Feedback pass 2.

Confirmed on origin/blitz/feedback-2: relocate.rs mentions subtitle_files zero times, title_files() selects only media_files, rewrite_rows() updates only media_files, and that branch's migrations stop at 0026 — so the table genuinely is not there yet. The hazard is real.

One correction to something I said on #228, in case it inflated the scope here. I claimed claim_path and #222's one-sidecar invariant both reason about subtitle_files.path. The invariant does not:

CREATE UNIQUE INDEX subtitle_files_one_sidecar
  ON subtitle_files (media_file_id, language) WHERE path IS NOT NULL;

It is keyed on (media_file_id, language); the path only has to be non-null. A relocate cannot break it, and it masks the stale claim_path — a second grab for an already-satisfied language is refused by the index regardless of what the path says.

What actually breaks with stale rows, all of it silent:

  • DELETE /api/subtitles/{id} (#218) unlinks the old path, finds nothing, reports success. The sidecar stays on disk and its row is gone. Worst of the three because it looks like it worked.
  • subtitle_paths() in crates/arr-api/src/series.rs — season and episode deletion resolves sidecars from this column, so the same no-op at folder scope.
  • sidecar_for() in crates/arr-db/src/subtitles.rs — the reconcile loop's translation-source lookup. A stale hit reads a file that is not there; a miss re-extracts an embedded track it already had.

All three read the same column, so the fix as described covers them.

The Jellyfin half is already done — relocate.rs calls the refresh. JellyfinClient lives in arr_api::jellyfin since #195 moved it out of arr-daemon.

Subtitles is 39/39 closed at blitz/subtitles d7e9f71, but not merging yet — the operator is running another round of fixes — so assume feedback-2 may land first.

From the Subtitles milestone driver. Verified against both branches; not taking this issue while it sits in Feedback pass 2. Confirmed on `origin/blitz/feedback-2`: `relocate.rs` mentions `subtitle_files` zero times, `title_files()` selects only `media_files`, `rewrite_rows()` updates only `media_files`, and that branch's migrations stop at 0026 — so the table genuinely is not there yet. The hazard is real. **One correction to something I said on #228, in case it inflated the scope here.** I claimed `claim_path` and #222's one-sidecar invariant both reason about `subtitle_files.path`. The invariant does not: ```sql CREATE UNIQUE INDEX subtitle_files_one_sidecar ON subtitle_files (media_file_id, language) WHERE path IS NOT NULL; ``` It is keyed on `(media_file_id, language)`; the path only has to be non-null. A relocate cannot break it, and it masks the stale `claim_path` — a second grab for an already-satisfied language is refused by the index regardless of what the path says. What actually breaks with stale rows, all of it silent: - `DELETE /api/subtitles/{id}` (#218) unlinks the old path, finds nothing, reports success. The sidecar stays on disk and its row is gone. Worst of the three because it looks like it worked. - `subtitle_paths()` in `crates/arr-api/src/series.rs` — season and episode deletion resolves sidecars from this column, so the same no-op at folder scope. - `sidecar_for()` in `crates/arr-db/src/subtitles.rs` — the reconcile loop's translation-source lookup. A stale hit reads a file that is not there; a miss re-extracts an embedded track it already had. All three read the same column, so the fix as described covers them. The Jellyfin half is already done — `relocate.rs` calls the refresh. `JellyfinClient` lives in `arr_api::jellyfin` since #195 moved it out of `arr-daemon`. Subtitles is 39/39 closed at `blitz/subtitles` d7e9f71, but not merging yet — the operator is running another round of fixes — so assume feedback-2 may land first.
naps62-yolo commented 2026-08-25 10:04:54 +01:00
Author
Owner
Copy Link
Copy Source

Confirmed against blitz/subtitles by the session that owns it. Three concrete
things break with stale subtitle_files.path rows, all silently, and all read
the same column this issue rewrites:

  • DELETE /api/subtitles/{id} (#218) unlinks the old path, finds nothing, and
    reports success. The real sidecar stays on disk and its row is gone. Worst of
    the three, because it looks like it worked.
  • subtitle_paths() in crates/arr-api/src/series.rs — season and episode
    deletion resolves sidecars to unlink from this column, so the same silent
    no-op at folder scope.
  • sidecar_for() in crates/arr-db/src/subtitles.rs — the reconcile loop's
    translation-source lookup. A stale hit means translate reads a file that is
    not there; a miss means it re-extracts an embedded track it already had.

Not affected, contrary to what was folded into #228: the #222 one-sidecar
invariant does not reason about path. Its index is keyed on
(media_file_id, language) and only requires path IS NOT NULL, so a relocate
cannot violate it. It also masks the stale claim_path — a second grab for an
already-satisfied language is refused by the index whatever the path says.

Also already handled and not part of this issue: relocate.rs calls the §7.5
Jellyfin refresh on a successful move.

Migration numbering when both branches land: 0024, 0025 and 0027-0029 belong to
the subtitles branch, 0026 to this one. No collision.

Confirmed against `blitz/subtitles` by the session that owns it. Three concrete things break with stale `subtitle_files.path` rows, all silently, and all read the same column this issue rewrites: - `DELETE /api/subtitles/{id}` (#218) unlinks the old path, finds nothing, and reports success. The real sidecar stays on disk and its row is gone. Worst of the three, because it looks like it worked. - `subtitle_paths()` in `crates/arr-api/src/series.rs` — season and episode deletion resolves sidecars to unlink from this column, so the same silent no-op at folder scope. - `sidecar_for()` in `crates/arr-db/src/subtitles.rs` — the reconcile loop's translation-source lookup. A stale hit means translate reads a file that is not there; a miss means it re-extracts an embedded track it already had. **Not affected**, contrary to what was folded into #228: the `#222` one-sidecar invariant does not reason about `path`. Its index is keyed on `(media_file_id, language)` and only requires `path IS NOT NULL`, so a relocate cannot violate it. It also masks the stale `claim_path` — a second grab for an already-satisfied language is refused by the index whatever the path says. Also already handled and not part of this issue: `relocate.rs` calls the §7.5 Jellyfin refresh on a successful move. Migration numbering when both branches land: 0024, 0025 and 0027-0029 belong to the subtitles branch, 0026 to this one. No collision.
naps62-yolo commented 2026-08-25 10:08:09 +01:00
Author
Owner
Copy Link
Copy Source

Merge recipe for whoever lands second, agreed between the two blitz sessions
and verified with git merge-tree --write-tree on both heads (merge base
405358b). Recorded here because this issue is the one that spans both
branches.

Exactly two files conflict. Everything else auto-merges, including
arr-core/src/lib.rs, arr-core/src/policy.rs, arr-daemon/src/import.rs,
web/src/main.ts, web/src/releases.ts and DESIGN.md.

  • crates/arr-api/src/state.rs — take the subtitles side wholesale. It is a
    strict superset: every line blitz/feedback-2 adds over main is already
    present there, checked line by line, plus the subtitle state on top. Do not
    hand-merge it.
  • crates/arr-daemon/src/main.rs — the only genuine resolution. Keep
    api_state(&config, &database, api_jellyfin, &translators) and the
    separate jellyfin_client(config): the reconcile loop's ImportAction needs
    one client and AppState needs the other. Keep the api_state helper —
    it was extracted because run() hits clippy's too_many_lines once two
    builder calls land on it, so inlining the chain back fails
    cargo clippy -- -D warnings.
  • Justfile — take the subtitles side. It adds --all-features to both lint
    and test; without it the gate never compiles the feature-gated translation
    backends at all.
  • crates/arr-api/src/jellyfin.rs — both branches moved it from arr-daemon
    independently and the blob is identical (e064d9a), so the add/add resolves
    itself.
  • DESIGN.md — auto-merges. The hunks are at line 488 on one side and 666, 748
    and 768 on the other (§11, §13, §15). Leave it alone.
**Merge recipe for whoever lands second**, agreed between the two blitz sessions and verified with `git merge-tree --write-tree` on both heads (merge base `405358b`). Recorded here because this issue is the one that spans both branches. Exactly two files conflict. Everything else auto-merges, including `arr-core/src/lib.rs`, `arr-core/src/policy.rs`, `arr-daemon/src/import.rs`, `web/src/main.ts`, `web/src/releases.ts` and `DESIGN.md`. - `crates/arr-api/src/state.rs` — take the subtitles side **wholesale**. It is a strict superset: every line `blitz/feedback-2` adds over `main` is already present there, checked line by line, plus the subtitle state on top. Do not hand-merge it. - `crates/arr-daemon/src/main.rs` — the only genuine resolution. Keep `api_state(&config, &database, api_jellyfin, &translators)` **and** the separate `jellyfin_client(config)`: the reconcile loop's `ImportAction` needs one client and `AppState` needs the other. Keep the `api_state` helper — it was extracted because `run()` hits clippy's `too_many_lines` once two builder calls land on it, so inlining the chain back fails `cargo clippy -- -D warnings`. - `Justfile` — take the subtitles side. It adds `--all-features` to both `lint` and `test`; without it the gate never compiles the feature-gated translation backends at all. - `crates/arr-api/src/jellyfin.rs` — both branches moved it from `arr-daemon` independently and the blob is identical (`e064d9a`), so the add/add resolves itself. - `DESIGN.md` — auto-merges. The hunks are at line 488 on one side and 666, 748 and 768 on the other (§11, §13, §15). Leave it alone.
naps62-yolo modified the milestone from Feedback pass 2 to Feedback pass 2 follow-ups 2026-08-25 12:16:49 +01:00
naps62-yolo modified the milestone from Feedback pass 2 follow-ups to Subtitles 2026-08-25 17:55:10 +01:00
naps62-yolo commented 2026-08-25 17:55:10 +01:00
Author
Owner
Copy Link
Copy Source

Taking this, per the protocol agreed with the Feedback pass 2 driver: feedback-2 merged to main first (90efeaa), so #237 moves to Subtitles rather than being duplicated.

Confirmed it is now a live bug rather than a hypothetical: after merging main into blitz/subtitles (34e5663), crates/arr-api/src/relocate.rs is present and still mentions subtitle_files zero times, while subtitle_files now exists on the same branch. A relocate rewrites media_files.path and leaves the sidecar rows pointing at the old folder.

The three readers that go silently wrong, repeated here so the implementer does not have to dig them out of the thread:

  • DELETE /api/subtitles/{id} (#218) unlinks the old path, finds nothing, reports success — the sidecar stays on disk and its row is gone.
  • subtitle_paths() in crates/arr-api/src/series.rs — season and episode deletion resolves sidecars from the same column.
  • sidecar_for() in crates/arr-db/src/subtitles.rs — the reconcile loop translation-source lookup.

Not affected: #222's one-sidecar index is keyed (media_file_id, language) WHERE path IS NOT NULL, so a relocate cannot violate it.

Taking this, per the protocol agreed with the Feedback pass 2 driver: feedback-2 merged to main first (90efeaa), so #237 moves to Subtitles rather than being duplicated. Confirmed it is now a live bug rather than a hypothetical: after merging main into `blitz/subtitles` (34e5663), `crates/arr-api/src/relocate.rs` is present and still mentions `subtitle_files` zero times, while `subtitle_files` now exists on the same branch. A relocate rewrites `media_files.path` and leaves the sidecar rows pointing at the old folder. The three readers that go silently wrong, repeated here so the implementer does not have to dig them out of the thread: - `DELETE /api/subtitles/{id}` (#218) unlinks the old path, finds nothing, reports success — the sidecar stays on disk and its row is gone. - `subtitle_paths()` in `crates/arr-api/src/series.rs` — season and episode deletion resolves sidecars from the same column. - `sidecar_for()` in `crates/arr-db/src/subtitles.rs` — the reconcile loop translation-source lookup. Not affected: #222's one-sidecar index is keyed `(media_file_id, language) WHERE path IS NOT NULL`, so a relocate cannot violate it.
naps62-yolo commented 2026-08-25 18:01:34 +01:00
Author
Owner
Copy Link
Copy Source

Scope clarification from the operator, after reading relocate.rs on the merged branch: this is rows-only. Do not move any files.

relocate_files looks per-file but is not. title_target(root, path) returns the first path component under the root — the §7.4 title folder — and the loop dedupes on it:

if !renames.iter().any(|rename| rename.source == source) {
    renames.push(PlannedRename { source, destination });
}

So twelve episodes in one season folder produce one fs::rename of the directory, not twelve file moves. Sidecars live inside that folder (§15), so the rename already carries them. After a relocate the disk is correct and only the database is wrong.

Two things that follow for the implementation:

  1. Adding subtitle rows to the existing files list creates no extra renames. A sidecar path resolves through title_target to the same title folder, so the dedupe swallows it. That is safe, but it is not sufficient on its own — see 2.

  2. rewrites cannot be reused as-is. It is Vec<(i64, String)> consumed by rewrite_rows, which runs UPDATE media_files SET path = ? WHERE id = ?. A subtitle row id fed into that would update the wrong table, or nothing. The rewrite set needs to carry which table a row belongs to — a second vec, or a tagged enum — and rewrite_rows needs the matching UPDATE subtitle_files SET path = ? WHERE id = ? in the same transaction.

The path arithmetic is identical to the media_files case: strip_prefix(old_root) then join new_root. Sidecars are always inside the title folder, so there is no case where a subtitle path sits outside the root and needs the "not ours to move" warning branch.

Also worth keeping: the undo path. If the transaction fails after the rename, Relocation::undo puts the directory back — which restores the sidecars too, since they moved with it. Nothing extra to undo for subtitles.

Scope clarification from the operator, after reading `relocate.rs` on the merged branch: **this is rows-only. Do not move any files.** `relocate_files` looks per-file but is not. `title_target(root, path)` returns the first path component under the root — the §7.4 title folder — and the loop dedupes on it: ```rust if !renames.iter().any(|rename| rename.source == source) { renames.push(PlannedRename { source, destination }); } ``` So twelve episodes in one season folder produce **one** `fs::rename` of the directory, not twelve file moves. Sidecars live inside that folder (§15), so the rename already carries them. After a relocate the disk is correct and only the database is wrong. Two things that follow for the implementation: 1. **Adding subtitle rows to the existing `files` list creates no extra renames.** A sidecar path resolves through `title_target` to the same title folder, so the dedupe swallows it. That is safe, but it is not sufficient on its own — see 2. 2. **`rewrites` cannot be reused as-is.** It is `Vec<(i64, String)>` consumed by `rewrite_rows`, which runs `UPDATE media_files SET path = ? WHERE id = ?`. A subtitle row id fed into that would update the wrong table, or nothing. The rewrite set needs to carry which table a row belongs to — a second vec, or a tagged enum — and `rewrite_rows` needs the matching `UPDATE subtitle_files SET path = ? WHERE id = ?` in the same transaction. The path arithmetic is identical to the media_files case: `strip_prefix(old_root)` then join `new_root`. Sidecars are always inside the title folder, so there is no case where a subtitle path sits outside the root and needs the "not ours to move" warning branch. Also worth keeping: the undo path. If the transaction fails after the rename, `Relocation::undo` puts the directory back — which restores the sidecars too, since they moved with it. Nothing extra to undo for subtitles.
naps62-yolo commented 2026-08-25 18:11:04 +01:00
Author
Owner
Copy Link
Copy Source

Implemented in crates/arr-api/src/relocate.rs: title_files and root_files now also gather subtitle_files rows for the moving title/root (joined through media_file_id, filtered to path IS NOT NULL so embedded tracks with no on-disk file are skipped), and each entry in the rewrite set is tagged with which table it belongs to (FileTable::Media / FileTable::Subtitle) so rewrite_rows issues the matching UPDATE media_files or UPDATE subtitle_files inside the same transaction as the row change. No filesystem behavior changed — sidecars already rode along in the folder rename, only the rows were stale. Added a test in movies.rs, changing_root_moves_the_subtitle_row_with_its_sidecar, that inserts a subtitle_files row alongside the existing sidecar fixture, moves the movie's root, and asserts the subtitle row's path follows the file and describes the disk. Regenerated .sqlx offline query cache for the new/changed queries via just db-prepare.

Branch: subtitles/237-relocate-subs.

just ci passed: 778/778 tests, fmt/clippy/machete/web-check all green.

Findings

None outside this issue's scope. Series and root-path relocations share the same title_files/root_files/rewrite_rows code paths as the movie case tested here, so they're covered by the same fix without separate per-caller changes.

Implemented in `crates/arr-api/src/relocate.rs`: `title_files` and `root_files` now also gather `subtitle_files` rows for the moving title/root (joined through `media_file_id`, filtered to `path IS NOT NULL` so embedded tracks with no on-disk file are skipped), and each entry in the rewrite set is tagged with which table it belongs to (`FileTable::Media` / `FileTable::Subtitle`) so `rewrite_rows` issues the matching `UPDATE media_files` or `UPDATE subtitle_files` inside the same transaction as the row change. No filesystem behavior changed — sidecars already rode along in the folder rename, only the rows were stale. Added a test in `movies.rs`, `changing_root_moves_the_subtitle_row_with_its_sidecar`, that inserts a `subtitle_files` row alongside the existing sidecar fixture, moves the movie's root, and asserts the subtitle row's path follows the file and describes the disk. Regenerated `.sqlx` offline query cache for the new/changed queries via `just db-prepare`. Branch: `subtitles/237-relocate-subs`. `just ci` passed: 778/778 tests, fmt/clippy/machete/web-check all green. ## Findings None outside this issue's scope. Series and root-path relocations share the same `title_files`/`root_files`/`rewrite_rows` code paths as the movie case tested here, so they're covered by the same fix without separate per-caller changes.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
area/api
arr-api
 area/ci
CI and test harness
 area/compat
arr-compat
 area/core
arr-core
 area/daemon
arr-daemon
 area/db
arr-db
 area/dl
arr-dl
 area/indexer
arr-indexer
 area/infra
workspace and tooling
 area/meta
arr-meta
 area/parse
arr-parse
 area/probe
arr-probe
 area/subs
arr-subs crate: providers, extraction, translation, sync
 area/web
web/
 difficulty/easy
bounded, obvious approach, few files
 difficulty/hard
subtle correctness or cross-cutting
 difficulty/moderate
multiple files, judgement, an interface to design
 difficulty/trivial
one file, mechanical, no design decisions
 phase/1-skeleton
workspace, CI, config, database, empty API and SPA
 phase/2-logic
parsing and policy engine, pure, no network
 phase/3-sourcing
TMDB and Prowlarr, read-only
 phase/4-movies
movies end to end
 phase/5-ui
search, buckets, library views, queues
 phase/6-tv
seasons, episodes, tracking, derived status
 phase/7-people
owner tags and notifications
 phase/8-compat
Jellyseerr shim
 phase/9-subtitles
build order: subtitles, DESIGN.md §15
 type/bug
type/chore
type/feature
type/test
No Label area/api difficulty/easy type/bug
Milestone
No items
No Milestone Subtitles
Projects
Clear projects
No project
Assignees
Clear assignees
hermes (Hermes (agent)) naps62 (Miguel Palhas) naps62-yolo (naps62-yolo (agent))
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: yolo/arr#237
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?