Compare commits

..
12 Commits
Author SHA1 Message Date
dtourolle 4bce81a800 chore(release): v0.11.6
🏗️ Build and Test JellyTau / Run Tests (push) Skipped
🏗️ Build and Test JellyTau / Android Compile Check (push) Skipped
🏗️ Build and Test JellyTau / Supply Chain (push) Successful in 4m4s
📱 Test APK / Build test APK (push) Successful in 48m6s
Publish Documentation / Build & publish docs to gitea-pages (push) Successful in 9m17s
Traceability Validation / Check Requirement Traces (push) Successful in 20s
Build & Release / Run Tests (push) Successful in 22m27s
Build & Release / Build Linux (push) Successful in 31m18s
Build & Release / Build Windows (push) Successful in 30m1s
Build & Release / Build Android (push) Successful in 45m53s
Build & Release / Create Release (push) Successful in 1m15s
Seven fixes from an audit of the stack's most fragile seams, each with a
test that fails without it. Two could take the app out entirely: an
interrupted database migration left it unable to launch at all, and an
unguarded panic at the Android JNI boundary aborted the process outright.

Frontend gates (bun run test/check/lint/format:check) were not run for
this release — node on the release machine is missing libada.so.3 and
exits 127. All changes are under src-tauri/; CI runs those gates.
2026-09-07 22:29:40 +02:00
dtourolle 65d3d912f7 fix(player): declare and enforce a lock hierarchy for PlayerController
The controller carries seventeen mutexes, reached from the MPV event loop,
JNI callbacks, sleep and autoplay timers, the session poller and every IPC
command. Nothing prevented two threads taking the same pair in opposite
orders, which deadlocks playback outright — and this subsystem has already
produced one deadlock.

No inversion exists today: the acquisitions really are scoped, and
`previous()` explicitly drops the backend guard before touching the queue.
That is the point. It holds by convention, convention is not checked, and
the failure it guards against is a frozen app with no error anywhere.

`LOCK_ORDER` writes the convention down, following the nesting the code
already relies on — `backend` before `queue` ("what is playing" before
"what is next"), `event_emitter` last because notifying the frontend must
never reach back for player state.

The tripwire only reports acquisitions that actually *overlap*, since two
locks taken one after another, each released before the next, cannot
deadlock. Verified by injecting a real inversion into `seek()`, which the
test located by line and rank.
2026-09-07 22:24:45 +02:00
dtourolle 192a8b3c67 fix(credentials): persist the fallback key instead of deriving an unstable one
The encrypted-file fallback derived its AES key from the hostname, a
hardcoded salt and `$USER`. Two problems, and the second is the one users
actually hit.

It was never secret. Every input is readable by anyone who can read the
ciphertext beside it, so the derivation bought nothing against the threat
its name implies. Calling the result "AES-256-GCM encrypted" oversold it.

And it was unstable. Renaming the machine, or launching from a context
where `$USER` is unset — a systemd user service, some desktop launchers —
changed the key and made every stored token undecryptable.
`load_credentials_file` reports a failed decrypt as "no stored
credentials", so this surfaced as being silently signed out with nothing
to explain it.

The key is now 32 random bytes persisted beside the credentials file, mode
0600, generated on first use. That is strictly better on both counts:
higher entropy, and it does not move when the machine does. It is still
obfuscation at rest rather than a secret — the key sits next to what it
opens — and the module docs now say so plainly instead of implying
otherwise. The keyring remains the only place a token is really protected.

The old derivation is kept solely to read a file written by an earlier
build; anything it opens is immediately rewritten under the persisted key,
so no one is signed out by the upgrade.

Verified against aarch64-linux-android as well as the host.
2026-09-07 22:24:45 +02:00
dtourolle 747ec0161c fix(download): stop an empty download completing and then hanging the player
Two halves of one failure, either of which is enough to produce an
offline item that never starts.

The worker marked a transfer `completed` without checking it produced
any bytes, so a server that answered 200 with no body — an error page, a
transcode that yielded nothing — renamed a zero-byte `.part` into place
and published it as available offline. That is worse than failing: the
retry budget never applies and the UI shows the item as ready.

The media server then answered a request for that file with a span of
`{ start: 0, end: 0 }`. `end` is inclusive, so `Span::len()` reported
**one** byte: the response declared `Content-Length: 1` and streamed
nothing, which Chromium's media loader waits on forever. The user sees a
downloaded item that just never plays, with nothing explaining why.

A zero-length file has no satisfiable range, so `span_for` now returns
`None` and the server answers 416. An empty transfer is rejected as a
network error, which keeps the `.part` for a resume and lets the existing
retry budget do its job.
2026-09-07 22:24:45 +02:00
dtourolle bc92eb4dea fix(repository): stop a slow cache read surfacing as a network error offline
The cache leg of a cache-first query had a hard 100 ms deadline that
cancelled the read and reported it as a miss. That conflates "the cache
has nothing" with "the cache was slow", and the two want opposite
answers: offline the server leg fails too, so browsing surfaced a network
error over cached content that was sitting on disk.

It is not a rare race. The database is a single SQLite connection behind
a single mutex, so a concurrent write — a sync drain, a bulk
save_to_cache, a thumbnail write — blocks every read for its duration,
and 100 ms is easily exceeded on phone storage. It also compounded:
`spawn_blocking` work is not cancellable, so an abandoned query still ran
and still held the mutex, making the next one slower.

The deadline now bounds only the *fast path*. A cache query that misses it
keeps running on its own task, and when the server leg fails the race
waits that query out instead of discarding it. A cache that answers in
time still short-circuits the server exactly as before, and when both
sides genuinely fail the server's error is still what the caller sees.

`parallel_race`/`race_with_refresh` no longer need `&self`, so they are
associated functions and directly testable without constructing a
repository.

Not addressed here: one connection behind one mutex makes `PRAGMA
journal_mode = WAL` inert, since reads and writes fully serialise
regardless. A read pool is an architecture change and wants a spec.
2026-09-07 22:24:45 +02:00
dtourolle c72ca86865 fix(storage): stop a panic poisoning the connection mutex for the whole session
`RusqliteService` is the path every async database operation in the app
takes, and all seven of its lock sites used a raw `.lock()`. A single
panic while that guard is held poisons the mutex, after which every
database call for the rest of the process returns "poisoned lock" — for
a database-backed app, the entire UI stops working until restart.

`utils::lock` exists to stop exactly this cascade, and `storage::Database`
already used `lock_safe()`. The busiest lock in the app was the one that
did not.

The test poisons the connection the way a panicking row mapper would and
asserts queries still serve.

The same raw-lock pattern remains at ~121 command-layer sites on the
`DatabaseWrapper`/`CredentialsWrapper` mutexes. Those degrade to a failed
command rather than a panic, and converting them is a mechanical sweep
better reviewed on its own.
2026-09-07 22:24:45 +02:00
dtourolle f9e1a8e69a fix(android): contain panics at the JNI boundary instead of aborting the process
Ten `extern "system"` callbacks are entered by the JVM on arbitrary
threads. A panic unwinding out of one crosses the FFI boundary, which
Rust answers by aborting: the app vanishes with no Java exception, no
attributable stack trace, and no crash report the user can send. For
callbacks that fire four times a second during playback that is the worst
available failure mode.

It was reachable. `nativeOnPositionUpdate` built a fallback Tokio runtime
with `Runtime::new().unwrap()` on threads that have none, and
`Runtime::new()` fails under exactly the fd exhaustion and thread-spawn
refusal Android subjects a media app to. That now logs and drops the
report — losing one progress report is recoverable, losing the app is not.

Every callback body is wrapped in `jni_guard`, which catches the unwind
and logs it. It is a backstop, not a licence to panic: a contained panic
still leaves whatever it interrupted half-done.

The guard lives in `player::jni_guard` rather than `player::android`
because that module is `cfg(target_os = "android")` and so never compiles
on the host — which is why its 1575 lines had no tests at all. A tripwire
test asserts every entry point wraps its body, so an eleventh callback
cannot reintroduce the defect; it reads the source, since exercising the
real boundary needs a JVM.

Verified with `cargo check --target aarch64-linux-android`.
2026-09-07 22:24:45 +02:00
dtourolle d4f80a4afa fix(storage): make each migration atomic so a partial failure can't brick the app
Migrations ran as bare `execute_batch` calls with the `_migrations` row
written afterwards. SQLite autocommits every statement, so a migration
that died partway — low disk, an OOM kill, the process dying mid-boot —
left its earlier statements applied and recorded nothing.

That is unrecoverable rather than merely untidy. `execute_batch` aborts
on the first error, so the retry on the next launch failed at statement 1
with "duplicate column name" and kept failing forever, and
`Database::open` turns a migration error into a `panic!` — the app never
started again and the only fix was clearing app data, losing downloads
and logins. Several migrations have exactly the shape that triggers it:
006 is three `ADD COLUMN`s, 003/005/024 are full table rebuilds.

Each migration now runs in one transaction with its `_migrations` row
committed inside it, so a migration is all-or-nothing and a retry is
always safe. Every migration is pure DDL/DML, which SQLite runs
transactionally; a `PRAGMA` or `VACUUM` added to one would not roll back.

`migrate()` delegates to a new `migrate_with()` so a test can inject a
deliberately-failing migration.
2026-09-07 22:24:45 +02:00
dtourolle 7b738002a0 fix(ci): move MIGRATION_025 above the test module
clippy's items_after_test_module fired on schema.rs: the new migration
const was appended to the end of the file, which is after the
#[cfg(test)] block added alongside migration 024.

    error: items after a test module
      --> src/storage/schema.rs:901:1

Only visible under --all-targets, which compiles the test target; the
--lib run I checked locally cannot see it. CI runs --all-targets, so it
failed there and nowhere else. Verified this time with the exact CI
invocation rather than a narrower one.
2026-09-07 22:24:45 +02:00
dtourolle c41b8ec896 fix(library): record which library a cached item came from
🏗️ Build and Test JellyTau / Run Tests (push) Failing after 13m40s
🏗️ Build and Test JellyTau / Android Compile Check (push) Skipped
🏗️ Build and Test JellyTau / Supply Chain (push) Successful in 43s
📱 Test APK / Build test APK (push) Successful in 49m21s
Publish Documentation / Build & publish docs to gitea-pages (push) Successful in 8m39s
Traceability Validation / Check Requirement Traces (push) Successful in 24s
"TV" and "Shows" showed identical contents, and so would any two
libraries of the same type.

save_to_cache bound library_id NULL on every row it wrote, so nothing in
the cache knew where an item came from. The only association available
was the collection_type/item_type taxonomy, and that is unable in
principle to tell two libraries of one type apart -- both are 'tvshows',
so every Series on the server satisfies either. DR-277 narrowed the
library clause, which stopped Books and Photos serving the whole server,
but no clause over that taxonomy could have fixed this.

The write path is the single choke point every cached row passes through
and it already knows the parent being browsed, so it now resolves the
owning library once per call: the parent itself when it is a library,
otherwise the library its parent item was filed under, which carries the
association down a hierarchy as it is browsed. Synthetic parents like
"favorites" match neither and stay NULL -- they are not a library and
span several.

This is what makes the taxonomy stop being load-bearing. Library types
nobody enumerated -- Books, Photos, Collections, mixed libraries with no
collection type at all -- are now scoped by the same link as everything
else rather than by whether someone remembered to add an arm for them.

Existing rows cannot be repaired locally, because the association was
never stored: migration 025 clears synced_at to force a re-fetch, the
same move MIGRATION_018 made for is_folder. Nothing is deleted --
downloads, favourites and playback positions live in other tables, and a
cleared synced_at only means "ask the server again".

The new tests seed through save_to_cache rather than inserting rows
directly, so they exercise the path that was actually broken.
2026-09-07 19:41:40 +02:00
dtourolle dea78b89b9 test(library): pin collections, both as a library and as an item
🏗️ Build and Test JellyTau / Run Tests (push) Successful in 24m24s
🏗️ Build and Test JellyTau / Supply Chain (push) Successful in 1m3s
📱 Test APK / Build test APK (push) Successful in 48m0s
Publish Documentation / Build & publish docs to gitea-pages (push) Successful in 7m55s
Traceability Validation / Check Requirement Traces (push) Successful in 23s
🏗️ Build and Test JellyTau / Android Compile Check (push) Successful in 6m26s
"Is Collections broken too?" deserved an answer from the suite rather
than from reading the query.

Both, and they behave differently. A Collections *library* was hit by the
same defect as Books and Photos -- unmapped collection_type, no
include_item_types, so the library clause matched every cached row -- and
is fixed by the same change. The unknown-type test now covers boxsets,
photos, homevideos and the empty collection_type Jellyfin sends for a
mixed library, instead of standing on books alone.

An individual collection is a different path and keeps working: a
BoxSet's members carry parent_id, which the cache does store, so they
match the ordinary parent link rather than the library clause. That is
worth its own test because narrowing the clause could plausibly have
taken collections with it, and "Collections is empty" would look
identical to the bug being fixed.
2026-09-07 00:27:51 +02:00
dtourolle 368935e6f4 fix(library): scope a library listing to that library
Opening a library that is not Music, Movies or TV served whatever
happened to be cached — films under Books, albums under Photos — rather
than the library's own contents.

The cached-browse query matched a library parent with an EXISTS that
never referenced the item:

    OR EXISTS (SELECT 1 FROM libraries l
               WHERE l.id = ? AND l.server_id = i.server_id)

It asks only whether a library with the requested id exists, so it is
true for every cached row the moment the parent is any library. The three
typed libraries concealed it because their landing pages pass
include_item_types, which narrowed the result to albums or films or
series; the generic library page passes none, so nothing narrowed it at
all.

`library_id` now decides wherever the cache kept one. That is the
server's own answer, and the only thing that can scope a library whose
type has no mapping (Books, Photos, Collections) or none at all — a
mixed library, where Jellyfin sends CollectionType null. The
collection_type/item_type taxonomy stays as the fallback for rows written
before the link was stored, and a library with neither matches nothing
and falls through to the server, which does know what is in it.

The taxonomy is now one macro shared with the downloaded listing. That
listing had the identical defect and it was fixed there alone (DR-167) —
the comment there even says the mapping "is needed in two places that
must agree", which was true of a third place nobody looked at.

One existing assertion changed rather than being worked around:
UT-206 expected a lib-2 album back from a lib-1 listing, which only held
because of this bug. It is about parameter binding order, so it keeps
testing exactly that, now with an album that is really in lib-1.
2026-09-07 00:11:20 +02:00
18 changed files with 2073 additions and 486 deletions
+78
View File
@@ -9,6 +9,84 @@ generated trace matrix lives in [docs/traceability.md](docs/traceability.md).
For how long each fixed defect had been shipping before it was found, see
[docs/defect-windows.md](docs/defect-windows.md).
## v0.11.6
Found by an audit of the stack's most fragile seams rather than by hitting them,
so most of these are faults that had not yet been reported — several could only
be reached on a bad day, and the worst of them only once.
### 🐛 Fixes
- **An interrupted update can no longer stop the app from ever opening again.**
Changes to the local database were applied one statement at a time with no way
to undo a half-finished one. If an update was interrupted partway — a full
disk, the phone reclaiming memory, the app being killed mid-launch — the
earlier statements stuck while nothing recorded that the change had happened.
On the next launch it started again from the beginning, immediately hit the
part that was already done, and gave up; and since the app treats a database
it cannot prepare as fatal, it stopped opening at all, on every launch, with
the only way out being to clear its data and lose downloads and sign-ins. Each
change is now all-or-nothing, so an interrupted one leaves no trace and the
next launch simply tries again. (UR-002 → DR-012)
- **The app no longer vanishes without trace when the player hits trouble.**
The parts of the Android player that report back into the app — position,
state changes, errors, the end of a track — had no protection around them, and
a failure inside one killed the whole app instantly: no error, no message, not
even a crash report worth sending. One such failure was reachable in ordinary
use, on the position report that fires four times a second: under memory
pressure the app could fail to build the small worker it needs to send that
report, and that alone was enough to take everything down. A dropped position
report is now just a dropped position report. (UR-005 → DR-052)
- **One internal failure no longer disables the whole app until it is restarted.**
Every part of the app that reads or writes local data shares a single gate to
it. If anything failed while holding that gate, the gate stayed jammed: from
then on every library page, download, setting and sign-in returned an error for
the rest of the session, and only quitting and reopening cleared it. The gate
now recovers instead of jamming. (UR-002 → DR-012)
- **Browsing offline no longer reports a network error over content already on
the device.** A read of local data was given a tenth of a second to answer and
otherwise abandoned and treated as "nothing stored". That is easily exceeded
on phone storage whenever something else is writing — a sync catching up, a
batch of artwork being saved — and offline, where there is no server to fall
back to, the result was a network error shown over a library that was sitting
on disk. Worse, the abandoned read kept running and kept the storage busy,
making the next one slower still. A slow read is now waited for rather than
thrown away, and a fast one still answers immediately as before. (UR-002 →
DR-013)
- **A download that arrived empty is no longer presented as ready to play.** If
the server answered a download with nothing at all — an error page, a
conversion that produced no output — the empty file was moved into place and
the item was marked available offline. Opening it then hung: the app's own
media server promised one byte of it and sent none, so the player waited
forever with nothing on screen to say why. An empty download is now treated as
the failure it is, keeping the partial file so it can resume, and a request for
an empty file gets an honest refusal instead of a promise. (UR-019, UR-071 →
DR-168, DR-137)
- **Renaming your computer no longer signs you out.** On systems without a
password manager, sign-in tokens are kept in a file whose key was rebuilt from
the machine's name and the current username each time the app started. Rename
the machine, or launch it from somewhere the username is not set, and the key
came out different, the file could no longer be read, and the app treated that
as never having been signed in — with nothing shown to explain it. The key is
now made once, kept, and unaffected by what the machine is called. Existing
saved sign-ins are carried over automatically. This file has never been a
substitute for a real password manager, and the app now says so plainly rather
than implying otherwise. (UR-012 → IR-014)
- **The player's internal locking is now checked rather than merely careful.**
The playback controller coordinates seventeen separate pieces of shared state
across the audio engine, the lock screen, timers and every screen in the app.
Nothing stopped two of them being taken in opposite orders by different parts
of the code, which freezes playback outright with no error anywhere — a fault
this part of the app has produced before. The correct order is now written down
and enforced automatically, so a future change cannot quietly reintroduce it.
No such fault existed; this keeps it that way. (UR-005 → DR-052)
## v0.11.5
### 🐛 Fixes
+7 -1
View File
@@ -474,6 +474,8 @@ Internal architecture, components, and application logic.
| DR-274 | Startup shows the picker only when the last-used profile has a PIN, or when more than one profile exists and the setting asks for it; otherwise it resumes exactly as before. The feature is invisible to a single-account install, which is what makes it safe to ship without a migration anyone has to think about | Auth | UR-082 | Proposed |
| DR-275 | Idle re-lock separates "the UI is locked" from "who is the active profile", so audio keeps playing and keeps reporting as the account that started it while the screen is locked. Lockscreen transport controls keep working untouched, because nothing on a lockscreen browses or starts new content — the locked UI refuses only what reaches past the current queue. The timer starts when playback stops rather than when the UI goes quiet, and unlocking to a *different* profile stops playback. It lives in Rust beside the player state machine: it needs authoritative playback state, and a frontend timer dies with the WebView on Android | Player | UR-083 | Proposed |
| DR-276 | The picker and PIN pad render an opaque `unlock_method` and an `UnlockOutcome` union the backend returns; the frontend never compares a PIN, counts an attempt, or infers that an account without a PIN is a child's. "Child account" is not modelled at all — a child profile is simply one with no PIN — so no role taxonomy is invented on either side of a boundary that has leaked taxonomy before | Frontend | UR-082, UR-083 | Proposed |
| DR-277 | A library listing is scoped to that library. The cached-browse query matched a library parent with an `EXISTS` that never referenced the item — it asked only whether a library with the requested id existed — so the clause was true for every cached row on the server. Music, Movies and TV concealed it because their landing pages pass `include_item_types`, which narrowed the result; the generic library page passes none, so opening Books, Photos, Collections or a mixed library served whatever happened to be cached. The stored `library_id` now decides wherever the cache kept one, because that is the server's own answer and the only thing able to scope a library whose type has no mapping or none at all; the `collection_type``item_type` taxonomy is the fallback for rows written before it was stored, and a library with neither matches nothing and falls through to the server. The taxonomy itself is now a single macro shared with the downloaded listing, which had the identical defect fixed in isolation (DR-167) while this path kept it | Repository | UR-007 | Done |
| DR-278 | Cached items record the library they came from. `save_to_cache` bound `library_id` NULL on every row it wrote, so the only association available was the `collection_type``item_type` taxonomy — which cannot distinguish two libraries of the *same* type (a server with "TV" and "Shows" served both the same contents) and says nothing about a library whose type it does not map. The write path is the single choke point every cached row passes through and it already knows the parent being browsed, so it resolves the owning library once per call: the parent itself when it is a library, otherwise the library its parent item was already filed under, which propagates the association down a hierarchy as it is browsed. Synthetic parents such as `favorites` match neither and stay NULL, since they are not a library and span several. Existing rows cannot be repaired locally — the association was never stored — so migration 025 clears `synced_at` to force a re-fetch, the same move MIGRATION_018 made for `is_folder`; the taxonomy fallback stays for one release while caches refill | Repository | UR-007 | Done |
| DR-198 | The webview runs under a real Content-Security-Policy, and the asset protocol is scoped to the one directory it still serves. `csp` was `null`, which disables CSP entirely: any script that reached the web layer — through a future `{@html}`, a dependency, or a devtools paste — would have inherited the whole IPC surface, and with it the user's session. `script-src 'self'` (Tauri injects a nonce for SvelteKit's inline bootstrap script at build time, so no `'unsafe-inline'` is needed) plus `object-src`/`frame-src 'none'` and `base-uri 'self'` is the part that is genuinely restrictive. `img-src`/`media-src`/`connect-src` cannot be: the Jellyfin origin is typed in by the user at run time and is commonly plain `http` on a LAN, so they allow `http:`/`https:` — a wide grant for *data*, but one that still bars `file:`, `filesystem:` and scripting schemes, and leaves `script-src` untouched. `style-src` keeps `'unsafe-inline'` because Svelte compiles `style="…"` attributes (including `app.html`'s `display: contents` wrapper) into markup; this is safe only while no `<style>` element survives into `index.html`, since a nonce there would make Tauri's injection outrank — and therefore void — `'unsafe-inline'`. `worker-src blob:` and `media-src blob:` are hls.js: it demuxes in a worker built from a blob and attaches MSE through `URL.createObjectURL`. `asset:` and `http://asset.localhost` are the same protocol under the two naming schemes `convertFileSrc` emits (custom scheme on Linux/macOS, `http` host on Windows/Android); `ipc:`/`http://ipc.localhost` is the invoke transport, which would otherwise be blocked by `connect-src`. A run-time CSP naming the server origin exactly was rejected: Tauri computes the header from immutable config when it serves the HTML, so it would mean rebuilding config and reloading the webview on every server change, for a policy the user can already point anywhere. The asset-protocol scope narrows from `$APPDATA/**` to `$APPDATA/thumbnails/**` — since DR-137 moved downloaded media to the loopback server, `imageCache` is the only `convertFileSrc` caller left, so the database and the encrypted-token fallback file no longer sit inside the grant | Security | UR-012, UR-071 | Done |
---
@@ -490,7 +492,7 @@ Internal architecture, components, and application logic.
| UR-004 | IR-003, IR-004, IR-008, IR-011 | DR-002, DR-004, DR-006, DR-129, DR-171, DR-176, DR-177, DR-181, DR-182, DR-183, DR-185, DR-188, DR-203, DR-265 |
| UR-005 | - | DR-001, DR-005, DR-009, DR-178, DR-179, DR-186, DR-193, DR-195 |
| UR-006 | IR-005, IR-006, IR-007, IR-008 | DR-200, DR-201 |
| UR-007 | IR-010 | DR-007, DR-008, DR-016, DR-257, DR-262 |
| UR-007 | IR-010 | DR-007, DR-008, DR-016, DR-257, DR-262, DR-277, DR-278 |
| UR-008 | IR-010 | DR-007, DR-011 |
| UR-009 | IR-009, IR-010, IR-011 | - |
| UR-010 | IR-012, IR-021 | DR-037, DR-059 |
@@ -816,6 +818,10 @@ Internal architecture, components, and application logic.
| UT-245 | A `timeupdate` is applied while the video is playing — the case that froze the position behind a PiP window — and still yields to an in-flight seek, a seek-bar drag, and an element with no current data | DR-265 | Done |
| UT-246 | Opening a PiP window disarms background audio, and a background signal arriving with the native PiP flag false is still treated as PiP while the frontend's latch says the window is open — without resurrecting one it has already seen close | DR-266 | Done |
| UT-247 | A library whose `collection_type` has no mapping — Books, Photos, a mixed library — does not return the server's films, albums and shows from cache | DR-277 | Done |
| UT-248 | Narrowing the library clause does not starve the libraries that do have landing pages: music, movies and TV each still list their own media and none of the others | DR-277 | Done |
| UT-249 | Opening an individual collection still lists its own children: a BoxSet's members are matched by the stored `parent_id`, not by the library clause, so narrowing that clause did not empty collections | DR-277 | Done |
| UT-250 | Two libraries of the same collection type are not interchangeable: seeded through the real cache write path, a "TV" and a "Shows" library each list their own series and not the other's | DR-278 | Done |
### Integration Tests
| Test ID | Test Description | Traces To | Status |
+1 -1
View File
@@ -1,6 +1,6 @@
{
"name": "jellytau",
"version": "0.11.5",
"version": "0.11.6",
"description": "A cross-platform Jellyfin client built with Tauri, SvelteKit and Rust.",
"author": "Duncan Tourolle <duncan@tourolle.paris>",
"license": "MIT",
+1 -1
View File
@@ -2209,7 +2209,7 @@ dependencies = [
[[package]]
name = "jellytau"
version = "0.11.5"
version = "0.11.6"
dependencies = [
"aes-gcm",
"argon2",
+1 -1
View File
@@ -4,7 +4,7 @@ name = "jellytau"
# `player-conformance`, and a second binary makes a bare `cargo run` —
# which `tauri dev` issues — ambiguous.
default-run = "jellytau"
version = "0.11.5"
version = "0.11.6"
description = "A cross-platform Jellyfin client"
authors = ["Duncan Tourolle <duncan@tourolle.paris>"]
license = "MIT"
+296 -50
View File
@@ -4,8 +4,19 @@
//! - Primary: System keyring (Secret Service on Linux, Keychain on macOS)
//! - Fallback: AES-256-GCM encrypted file when keyring unavailable
//!
//! The fallback is less secure as the encryption key is derived from machine
//! identifiers, but provides functionality on headless systems.
//! The fallback is **obfuscation at rest, not a secret**: its key sits in a file
//! beside the ciphertext, so anyone who can read one can read the other. It
//! exists so headless systems keep working, and the keyring remains the only
//! place a token is actually protected.
//!
//! The key used to be *derived* from the hostname, `$USER` and a hardcoded salt.
//! That was no more secret — those are readable by anyone who can read the file
//! — and it was unstable: renaming the machine, or launching from a context
//! where `$USER` is unset, changed the key and made every stored token
//! undecryptable. `load_credentials_file` treats a failed decrypt as "no stored
//! credentials", so that surfaced as being silently signed out rather than as an
//! error. The key is now random and persisted, and the old derivation is kept
//! only to migrate a file written before this change.
//!
//! TRACES: UR-012 | IR-014
@@ -25,6 +36,119 @@ const SERVICE_NAME: &str = "com.dtourolle.jellytau";
const CREDENTIALS_FILENAME: &str = "credentials.enc";
/// Key file for the encrypted-file fallback, beside the credentials it opens.
const KEY_FILENAME: &str = "credentials.key";
/// Load the fallback encryption key, creating it on first use.
///
/// Random rather than derived. A derived key was no more secret — its inputs
/// (hostname, `$USER`, a hardcoded salt) are readable by anyone who can read
/// the ciphertext — and it silently changed when the machine was renamed or
/// `$USER` was unset, which read to the user as being signed out for no reason.
///
/// If the key cannot be persisted the process still gets a usable key for this
/// run; credentials written under it simply will not be readable next launch,
/// which is the same outcome as today and better than refusing to store a token.
///
/// TRACES: UR-012 | IR-014 | UT-014
fn load_or_create_key(path: &std::path::Path) -> [u8; 32] {
if let Ok(existing) = fs::read(path) {
if existing.len() == 32 {
let mut key = [0u8; 32];
key.copy_from_slice(&existing);
return key;
}
warn!(
"Fallback key at {:?} is {} bytes, not 32; replacing it. Credentials \
written under the old key will need signing in again.",
path,
existing.len()
);
}
let mut key = [0u8; 32];
if getrandom::getrandom(&mut key).is_err() {
warn!("No system randomness for the fallback key; deriving one for this run");
return CredentialStore::derive_legacy_encryption_key();
}
if let Some(parent) = path.parent() {
let _ = fs::create_dir_all(parent);
}
match fs::write(path, key) {
Ok(()) => restrict_to_owner(path),
Err(e) => warn!(
"Could not persist the fallback key at {:?} ({}); credentials stored \
this run will not be readable next launch",
path, e
),
}
key
}
/// Make a key file owner-readable only. Best effort — a filesystem without
/// Unix permissions is not a reason to fail.
fn restrict_to_owner(path: &std::path::Path) {
#[cfg(unix)]
{
use std::os::unix::fs::PermissionsExt;
if let Err(e) = fs::set_permissions(path, fs::Permissions::from_mode(0o600)) {
warn!("Could not restrict permissions on {:?}: {}", path, e);
}
}
#[cfg(not(unix))]
let _ = path;
}
/// Decrypt `encrypted` with `key`.
///
/// TRACES: UR-012 | IR-014 | UT-014
fn decrypt_with(key: &[u8; 32], encrypted: &str) -> Result<String, CredentialError> {
let combined = BASE64
.decode(encrypted)
.map_err(|e| CredentialError::Encryption(e.to_string()))?;
if combined.len() < 12 {
return Err(CredentialError::Encryption(
"Invalid encrypted data".to_string(),
));
}
let (nonce_bytes, ciphertext) = combined.split_at(12);
let nonce = Nonce::from_slice(nonce_bytes);
let cipher =
Aes256Gcm::new_from_slice(key).map_err(|e| CredentialError::Encryption(e.to_string()))?;
let plaintext = cipher
.decrypt(nonce, ciphertext)
.map_err(|e| CredentialError::Encryption(e.to_string()))?;
String::from_utf8(plaintext).map_err(|e| CredentialError::Encryption(e.to_string()))
}
/// Encrypt `plaintext` with `key`, prepending a fresh random nonce.
///
/// TRACES: UR-012 | IR-014 | UT-014
fn encrypt_with(key: &[u8; 32], plaintext: &str) -> Result<String, CredentialError> {
let cipher =
Aes256Gcm::new_from_slice(key).map_err(|e| CredentialError::Encryption(e.to_string()))?;
let mut nonce_bytes = [0u8; 12];
getrandom::getrandom(&mut nonce_bytes)
.map_err(|e| CredentialError::Encryption(e.to_string()))?;
let nonce = Nonce::from_slice(&nonce_bytes);
let ciphertext = cipher
.encrypt(nonce, plaintext.as_bytes())
.map_err(|e| CredentialError::Encryption(e.to_string()))?;
let mut combined = nonce_bytes.to_vec();
combined.extend(ciphertext);
Ok(BASE64.encode(&combined))
}
/// Result of a credential storage operation
#[derive(Debug)]
pub enum CredentialResult {
@@ -66,15 +190,21 @@ pub struct CredentialStore {
using_keyring: bool,
/// Path to the encrypted credentials file (fallback)
credentials_path: PathBuf,
/// Encryption key for file fallback (derived from machine ID)
/// Encryption key for the file fallback. Random and persisted, so it does
/// not change when the machine is renamed.
encryption_key: [u8; 32],
/// The pre-existing derivation, retained only to read a file written before
/// the key was persisted. Anything decrypted with it is rewritten under
/// `encryption_key`.
legacy_key: [u8; 32],
}
impl CredentialStore {
/// Create a new credential store, detecting the best available backend
pub fn new() -> Self {
let credentials_path = Self::get_credentials_path();
let encryption_key = Self::derive_encryption_key();
let encryption_key = load_or_create_key(&Self::get_key_path());
let legacy_key = Self::derive_legacy_encryption_key();
// Test if keyring is available by trying a dummy operation
let using_keyring = Self::test_keyring_available();
@@ -93,6 +223,7 @@ impl CredentialStore {
using_keyring,
credentials_path,
encryption_key,
legacy_key,
}
}
@@ -406,6 +537,16 @@ impl CredentialStore {
// --- Encrypted file backend ---
/// Where the fallback key lives: beside the credentials file, so the two
/// travel together and a restore that brings one brings the other.
fn get_key_path() -> PathBuf {
if let Some(proj_dirs) = ProjectDirs::from("com", "dtourolle", "jellytau") {
proj_dirs.data_dir().join(KEY_FILENAME)
} else {
PathBuf::from(KEY_FILENAME)
}
}
fn get_credentials_path() -> PathBuf {
if let Some(proj_dirs) = ProjectDirs::from("com", "dtourolle", "jellytau") {
proj_dirs.data_dir().join(CREDENTIALS_FILENAME)
@@ -414,7 +555,14 @@ impl CredentialStore {
}
}
fn derive_encryption_key() -> [u8; 32] {
/// The key derivation used before keys were persisted.
///
/// Kept **only** so a credentials file written by an older build can still
/// be read once and rewritten under the persisted key. Never used to
/// encrypt. See the module docs for why it was replaced.
///
/// TRACES: UR-012 | IR-014
fn derive_legacy_encryption_key() -> [u8; 32] {
// Derive a key from machine-specific identifiers
// This is less secure than a true keyring but provides some protection
let mut hasher = Sha256::new();
@@ -490,7 +638,7 @@ impl CredentialStore {
return Ok(serde_json::json!({}));
}
let decrypted = match self.decrypt(&encrypted_data) {
let (decrypted, from_legacy_key) = match self.decrypt_migrating(&encrypted_data) {
Ok(decrypted) => decrypted,
Err(e) => {
warn!(
@@ -505,7 +653,19 @@ impl CredentialStore {
};
match serde_json::from_str(&decrypted) {
Ok(value) => Ok(value),
Ok(value) => {
// Rewrite under the persisted key so the legacy derivation is
// never needed again.
if from_legacy_key {
if let Err(e) = self.save_credentials_file(&value) {
warn!(
"Could not rewrite credentials under the persisted key: {}",
e
);
}
}
Ok(value)
}
Err(e) => {
warn!(
"Credentials file at {:?} decrypted to invalid JSON ({}); \
@@ -531,48 +691,27 @@ impl CredentialStore {
}
fn encrypt(&self, plaintext: &str) -> Result<String, CredentialError> {
let cipher = Aes256Gcm::new_from_slice(&self.encryption_key)
.map_err(|e| CredentialError::Encryption(e.to_string()))?;
// Generate a random nonce
let mut nonce_bytes = [0u8; 12];
getrandom::getrandom(&mut nonce_bytes)
.map_err(|e| CredentialError::Encryption(e.to_string()))?;
let nonce = Nonce::from_slice(&nonce_bytes);
let ciphertext = cipher
.encrypt(nonce, plaintext.as_bytes())
.map_err(|e| CredentialError::Encryption(e.to_string()))?;
// Prepend nonce to ciphertext and encode as base64
let mut combined = nonce_bytes.to_vec();
combined.extend(ciphertext);
Ok(BASE64.encode(&combined))
encrypt_with(&self.encryption_key, plaintext)
}
fn decrypt(&self, encrypted: &str) -> Result<String, CredentialError> {
let combined = BASE64
.decode(encrypted)
.map_err(|e| CredentialError::Encryption(e.to_string()))?;
if combined.len() < 12 {
return Err(CredentialError::Encryption(
"Invalid encrypted data".to_string(),
));
/// Decrypt with the current key, falling back to the legacy derivation.
///
/// Returns the plaintext and whether the legacy key was what opened it, so
/// the caller can rewrite the file under the current key and stop depending
/// on a derivation that changes when the machine is renamed.
///
/// TRACES: UR-012 | IR-014 | UT-014
fn decrypt_migrating(&self, encrypted: &str) -> Result<(String, bool), CredentialError> {
match decrypt_with(&self.encryption_key, encrypted) {
Ok(plaintext) => Ok((plaintext, false)),
Err(current_err) => match decrypt_with(&self.legacy_key, encrypted) {
Ok(plaintext) => {
info!("Credentials were written under the legacy derived key; rewriting them");
Ok((plaintext, true))
}
Err(_) => Err(current_err),
},
}
let (nonce_bytes, ciphertext) = combined.split_at(12);
let nonce = Nonce::from_slice(nonce_bytes);
let cipher = Aes256Gcm::new_from_slice(&self.encryption_key)
.map_err(|e| CredentialError::Encryption(e.to_string()))?;
let plaintext = cipher
.decrypt(nonce, ciphertext)
.map_err(|e| CredentialError::Encryption(e.to_string()))?;
String::from_utf8(plaintext).map_err(|e| CredentialError::Encryption(e.to_string()))
}
fn save_to_file(&self, user_id: &str, token: &str) -> Result<(), CredentialError> {
@@ -884,6 +1023,103 @@ pub use android_keystore::{
#[cfg(test)]
mod tests {
/// The fallback key must be the same on every launch.
///
/// It used to be derived from the hostname, `$USER` and a static salt.
/// Renaming the machine — or launching from a context where `$USER` is
/// unset, such as a systemd user service — changed the key, and
/// `load_credentials_file` reports a failed decrypt as "no stored
/// credentials". The user was silently signed out with nothing to explain it.
///
/// TRACES: UR-012 | IR-014 | UT-014
#[test]
fn the_fallback_key_is_stable_across_processes() {
let dir = std::env::temp_dir().join(format!("jellytau-key-{}", std::process::id()));
let path = dir.join("credentials.key");
let _ = fs::remove_file(&path);
let first = load_or_create_key(&path);
let second = load_or_create_key(&path);
assert_eq!(first, second, "the key must not change between launches");
assert_ne!(first, [0u8; 32], "the key must be real randomness");
let _ = fs::remove_dir_all(&dir);
}
/// Two installs must not share a key.
///
/// TRACES: UR-012 | IR-014 | UT-014
#[test]
fn separate_installs_get_separate_keys() {
let base = std::env::temp_dir().join(format!("jellytau-keys-{}", std::process::id()));
let a = load_or_create_key(&base.join("a").join("credentials.key"));
let b = load_or_create_key(&base.join("b").join("credentials.key"));
assert_ne!(a, b);
let _ = fs::remove_dir_all(&base);
}
/// A credentials file written under the old derived key must still open.
///
/// TRACES: UR-012 | IR-014 | UT-014
#[test]
fn credentials_written_under_the_legacy_key_still_decrypt() {
let legacy = CredentialStore::derive_legacy_encryption_key();
let mut persisted = [0u8; 32];
getrandom::getrandom(&mut persisted).unwrap();
assert_ne!(legacy, persisted);
let blob = encrypt_with(&legacy, r#"{"user-1":"token-abc"}"#).unwrap();
let store = CredentialStore {
using_keyring: false,
credentials_path: PathBuf::from("/nonexistent/credentials.enc"),
encryption_key: persisted,
legacy_key: legacy,
};
let (plaintext, migrated) = store
.decrypt_migrating(&blob)
.expect("a file written under the legacy key must still be readable");
assert_eq!(plaintext, r#"{"user-1":"token-abc"}"#);
assert!(migrated, "the caller must know to rewrite it");
}
/// The current key is tried first and needs no migration.
///
/// TRACES: UR-012 | IR-014 | UT-014
#[test]
fn credentials_under_the_current_key_are_not_flagged_for_migration() {
let mut persisted = [0u8; 32];
getrandom::getrandom(&mut persisted).unwrap();
let blob = encrypt_with(&persisted, "hello").unwrap();
let store = CredentialStore {
using_keyring: false,
credentials_path: PathBuf::from("/nonexistent/credentials.enc"),
encryption_key: persisted,
legacy_key: [7u8; 32],
};
let (plaintext, migrated) = store.decrypt_migrating(&blob).unwrap();
assert_eq!(plaintext, "hello");
assert!(!migrated);
}
/// A blob under neither key fails rather than returning something wrong.
///
/// TRACES: UR-012 | IR-014 | UT-014
#[test]
fn an_unreadable_blob_is_an_error() {
let blob = encrypt_with(&[1u8; 32], "secret").unwrap();
let store = CredentialStore {
using_keyring: false,
credentials_path: PathBuf::from("/nonexistent/credentials.enc"),
encryption_key: [2u8; 32],
legacy_key: [3u8; 32],
};
assert!(store.decrypt_migrating(&blob).is_err());
}
use super::*;
/// Build a store pinned to the encrypted-file backend with an explicit key,
@@ -894,6 +1130,9 @@ mod tests {
using_keyring: false,
credentials_path,
encryption_key,
// A distinct legacy key, so "same file, different machine key" stays
// undecryptable rather than being opened by the migration path.
legacy_key: [0xABu8; 32],
}
}
@@ -959,15 +1198,22 @@ mod tests {
let plaintext = "test-access-token-12345";
let encrypted = store.encrypt(plaintext).unwrap();
let decrypted = store.decrypt(&encrypted).unwrap();
let (decrypted, _) = store.decrypt_migrating(&encrypted).unwrap();
assert_eq!(plaintext, decrypted);
}
/// The legacy derivation must stay deterministic *within a machine*, or the
/// one-time migration of an old credentials file cannot read it.
///
/// Its instability *across* machine states is exactly why it no longer
/// encrypts anything — see `the_fallback_key_is_stable_across_processes`.
///
/// TRACES: UR-012 | IR-014 | UT-014
#[test]
fn test_derive_encryption_key_is_deterministic() {
let key1 = CredentialStore::derive_encryption_key();
let key2 = CredentialStore::derive_encryption_key();
fn test_legacy_derivation_is_deterministic_for_migration() {
let key1 = CredentialStore::derive_legacy_encryption_key();
let key2 = CredentialStore::derive_legacy_encryption_key();
assert_eq!(key1, key2);
}
}
+44
View File
@@ -183,6 +183,16 @@ impl DownloadWorker {
.await
.map_err(|e| DownloadError::FileSystem(e.to_string()))?;
// A media file is never legitimately empty, and completing one is worse
// than failing: the row goes `completed`, the item shows as available
// offline, and playback then stalls on a file with nothing in it. A
// server that answered 200 with no body — an error page, a transcode
// that produced nothing — used to land here. Treat it as the network
// failure it is so the retry budget applies and the `.part` is kept.
if let Some(reason) = rejects_as_empty(downloaded) {
return Err(DownloadError::Network(reason.to_string()));
}
// Move from .part to final location
fs::rename(&temp_path, &task.target_path)
.await
@@ -229,6 +239,23 @@ pub fn resume_offset(existing_bytes: u64, status: u16) -> u64 {
}
}
/// Why a finished transfer must not be accepted, if it must not be.
///
/// A media file is never legitimately empty, and *completing* an empty one is
/// worse than failing: the row goes `completed`, the item shows as available
/// offline, and playback later stalls on a file with nothing in it. A server
/// that answered 200 with no body — an error page, a transcode that produced
/// nothing — used to land exactly there.
///
/// Reported as a network error so the existing retry budget applies and the
/// `.part` file is kept for a resume.
///
/// TRACES: UR-019 | DR-168 | UT-168
pub fn rejects_as_empty(downloaded: u64) -> Option<&'static str> {
(downloaded == 0)
.then_some("server sent an empty body; refusing to complete a zero-byte download")
}
/// The partial-download sidecar for `target`.
///
/// **Appends** `.part` rather than replacing the extension. The worker used
@@ -305,6 +332,23 @@ impl std::error::Error for DownloadError {}
mod tests {
use super::*;
/// A transfer that produced no bytes must never be marked complete.
///
/// Completing it publishes an empty file as playable offline; the media
/// server then answers a request for it with a 416 and the item simply
/// never starts, with nothing in the UI explaining why.
///
/// TRACES: UR-019 | DR-168 | UT-168
#[test]
fn test_a_zero_byte_transfer_is_rejected_rather_than_completed() {
assert!(
rejects_as_empty(0).is_some(),
"a zero-byte download must not be completed"
);
assert!(rejects_as_empty(1).is_none());
assert!(rejects_as_empty(4 * 1024 * 1024).is_none());
}
/// The bitrate-download corruption: a transcode ignores `Range` and answers
/// `200` with the whole stream. Appending that to the bytes already on disk
/// duplicated them, so every retry grew the file past its real size and left
+41 -1
View File
@@ -326,8 +326,12 @@ impl Span {
///
/// TRACES: UR-071 | DR-137 | UT-127
pub fn span_for(range: Option<&str>, len: u64) -> Option<Span> {
// A zero-length file has no byte to serve. `end` is inclusive, so the
// shortest span this type can express is one byte — returning one for an
// empty file declared `Content-Length: 1` and then streamed nothing, which
// Chromium's media loader waits on forever. 416 says so honestly instead.
if len == 0 {
return Some(Span { start: 0, end: 0 });
return None;
}
let last = len - 1;
let first_chunk = Span {
@@ -442,6 +446,42 @@ fn content_type(path: &Path, head: &[u8]) -> &'static str {
mod tests {
use super::*;
/// An empty file must not be answered with a span that promises a byte.
///
/// `Span::len()` is `end + 1 - start`, so the `Span { start: 0, end: 0 }`
/// that a zero-length file used to produce reported a length of **one**.
/// The response then declared `Content-Length: 1` and streamed nothing,
/// which Chromium's media loader waits on forever — reaching the user as a
/// downloaded item that never starts. A zero-byte file has no satisfiable
/// range, so 416 is the honest answer.
///
/// TRACES: UR-071 | DR-137 | UT-127
#[test]
fn test_span_for_an_empty_file_is_unsatisfiable() {
assert!(
span_for(None, 0).is_none(),
"a zero-length file has no byte to serve"
);
assert!(span_for(Some("bytes=0-"), 0).is_none());
assert!(span_for(Some("bytes=0-100"), 0).is_none());
}
/// Whatever a span says, its length must match the bytes that follow it.
///
/// TRACES: UR-071 | DR-137 | UT-127
#[test]
fn test_span_len_never_exceeds_the_file() {
for len in [0u64, 1, 2, 4095, CHUNK_LEN, CHUNK_LEN + 1] {
if let Some(span) = span_for(None, len) {
assert!(
span.len() <= len,
"span for a {len}-byte file claims {} bytes",
span.len()
);
}
}
}
/// The whole point: a request with no `Range` must still come back bounded.
/// That is the case Tauri's asset protocol answers with the entire file —
/// the read Chromium abandoned after 31s.
+70 -4
View File
@@ -3,6 +3,7 @@
//! This module provides a `PlayerBackend` implementation using Android's ExoPlayer
//! through JNI calls to Kotlin code.
use super::jni_guard::jni_guard;
use crate::utils::lock::MutexSafe;
use log::debug;
use std::sync::{Arc, Mutex, OnceLock};
@@ -677,6 +678,9 @@ pub extern "system" fn Java_com_dtourolle_jellytau_player_JellyTauPlayer_nativeO
position: jdouble,
duration: jdouble,
) {
jni_guard(
"Java_com_dtourolle_jellytau_player_JellyTauPlayer_nativeOnPositionUpdate",
|| {
// Debug: Log every 10th update to avoid spam
static mut UPDATE_COUNTER: u32 = 0;
unsafe {
@@ -713,6 +717,8 @@ pub extern "system" fn Java_com_dtourolle_jellytau_player_JellyTauPlayer_nativeO
// playing, but guard on the stored state anyway. Mirrors the MPV backend's
// progress loop; both share the same EventThrottler (every 30s per item).
report_android_progress(position);
},
);
}
/// Report throttled playback progress to Jellyfin from the Android position
@@ -774,8 +780,17 @@ fn report_android_progress(position: f64) {
handle.spawn(spawn_report());
} else {
std::thread::spawn(move || {
let rt = tokio::runtime::Runtime::new().unwrap();
rt.block_on(spawn_report());
// Not `unwrap()`: this runs on a JNI thread, and `Runtime::new()`
// fails under the fd exhaustion and thread-spawn refusal Android
// subjects a media app to. The panic used to unwind out of the
// `extern "system"` caller and abort the process — losing one
// progress report is recoverable, losing the app is not.
match tokio::runtime::Runtime::new() {
Ok(rt) => rt.block_on(spawn_report()),
Err(e) => log::error!(
"[Android] No runtime available to report progress; dropping it: {e}"
),
}
});
}
@@ -790,6 +805,9 @@ pub extern "system" fn Java_com_dtourolle_jellytau_player_JellyTauPlayer_nativeO
state: JString,
media_id: JString,
) {
jni_guard(
"Java_com_dtourolle_jellytau_player_JellyTauPlayer_nativeOnStateChanged",
|| {
let state_str: String = env.get_string(&state).map(|s| s.into()).unwrap_or_default();
let media_id_opt: Option<String> = if media_id.is_null() {
@@ -835,6 +853,8 @@ pub extern "system" fn Java_com_dtourolle_jellytau_player_JellyTauPlayer_nativeO
media_id: media_id_opt,
});
}
},
);
}
/// Called when media has finished loading.
@@ -844,6 +864,9 @@ pub extern "system" fn Java_com_dtourolle_jellytau_player_JellyTauPlayer_nativeO
_class: JClass,
duration: jdouble,
) {
jni_guard(
"Java_com_dtourolle_jellytau_player_JellyTauPlayer_nativeOnMediaLoaded",
|| {
if let Some(state) = SHARED_STATE.get() {
let mut state = state.lock_safe();
state.duration = Some(duration);
@@ -853,6 +876,8 @@ pub extern "system" fn Java_com_dtourolle_jellytau_player_JellyTauPlayer_nativeO
if let Some(emitter) = EVENT_EMITTER.get() {
emitter.emit(PlayerStatusEvent::MediaLoaded { duration });
}
},
);
}
/// Called when playback reaches the end.
@@ -861,6 +886,9 @@ pub extern "system" fn Java_com_dtourolle_jellytau_player_JellyTauPlayer_nativeO
_env: JNIEnv,
_class: JClass,
) {
jni_guard(
"Java_com_dtourolle_jellytau_player_JellyTauPlayer_nativeOnPlaybackEnded",
|| {
log::info!("[ExoPlayer] Playback ended - processing autoplay decision");
// Get player controller and handle autoplay decision
@@ -912,13 +940,19 @@ pub extern "system" fn Java_com_dtourolle_jellytau_player_JellyTauPlayer_nativeO
queue.items().len()
)
};
log::debug!("[Autoplay] Queue state after next(): {}", queue_info);
log::debug!(
"[Autoplay] Queue state after next(): {}",
queue_info
);
// Emit queue changed event so frontend updates UI with new current track
ctrl.emit_queue_changed();
}
Err(e) => {
log::error!("[Autoplay] Failed to advance to next track: {}", e);
log::error!(
"[Autoplay] Failed to advance to next track: {}",
e
);
// Emit PlaybackEnded event on error
if let Some(emitter) = EVENT_EMITTER.get() {
emitter.emit(PlayerStatusEvent::PlaybackEnded);
@@ -999,6 +1033,8 @@ pub extern "system" fn Java_com_dtourolle_jellytau_player_JellyTauPlayer_nativeO
emitter.emit(PlayerStatusEvent::PlaybackEnded);
}
}
},
);
}
/// Called when buffering state changes.
@@ -1008,11 +1044,16 @@ pub extern "system" fn Java_com_dtourolle_jellytau_player_JellyTauPlayer_nativeO
_class: JClass,
percent: jint,
) {
jni_guard(
"Java_com_dtourolle_jellytau_player_JellyTauPlayer_nativeOnBuffering",
|| {
if let Some(emitter) = EVENT_EMITTER.get() {
emitter.emit(PlayerStatusEvent::Buffering {
percent: percent as u8,
});
}
},
);
}
/// Called when a playback error occurs.
@@ -1023,6 +1064,9 @@ pub extern "system" fn Java_com_dtourolle_jellytau_player_JellyTauPlayer_nativeO
message: JString,
recoverable: jboolean,
) {
jni_guard(
"Java_com_dtourolle_jellytau_player_JellyTauPlayer_nativeOnError",
|| {
let message_str: String = env
.get_string(&message)
.map(|s| s.into())
@@ -1084,6 +1128,8 @@ pub extern "system" fn Java_com_dtourolle_jellytau_player_JellyTauPlayer_nativeO
recoverable,
});
}
},
);
}
/// Called when volume changes.
@@ -1094,6 +1140,9 @@ pub extern "system" fn Java_com_dtourolle_jellytau_player_JellyTauPlayer_nativeO
volume: jfloat,
muted: jboolean,
) {
jni_guard(
"Java_com_dtourolle_jellytau_player_JellyTauPlayer_nativeOnVolumeChanged",
|| {
if let Some(state) = SHARED_STATE.get() {
state.lock_safe().volume = volume;
}
@@ -1104,6 +1153,8 @@ pub extern "system" fn Java_com_dtourolle_jellytau_player_JellyTauPlayer_nativeO
muted: muted != 0,
});
}
},
);
}
// JNI callback for MediaSession commands from JellyTauPlaybackService
@@ -1127,6 +1178,9 @@ pub extern "system" fn Java_com_dtourolle_jellytau_player_JellyTauPlaybackServic
_class: JClass,
command: JString,
) {
jni_guard(
"Java_com_dtourolle_jellytau_player_JellyTauPlaybackService_nativeOnMediaCommand",
|| {
let command_str: String = env
.get_string(&command)
.map(|s| s.into())
@@ -1135,6 +1189,8 @@ pub extern "system" fn Java_com_dtourolle_jellytau_player_JellyTauPlaybackServic
if let Some(handler) = MEDIA_COMMAND_HANDLER.get() {
handler.on_command(&command_str);
}
},
);
}
/// JNI callback from JellyTauPlaybackService when volume buttons are pressed in remote mode.
@@ -1148,6 +1204,9 @@ pub extern "system" fn Java_com_dtourolle_jellytau_player_JellyTauPlaybackServic
command: JString,
volume: jint,
) {
jni_guard(
"Java_com_dtourolle_jellytau_player_JellyTauPlaybackService_nativeOnRemoteVolumeChange",
|| {
let command_str: String = env
.get_string(&command)
.map(|s| s.into())
@@ -1156,6 +1215,8 @@ pub extern "system" fn Java_com_dtourolle_jellytau_player_JellyTauPlaybackServic
if let Some(handler) = REMOTE_VOLUME_HANDLER.get() {
handler.on_remote_volume_change(&command_str, volume as i32);
}
},
);
}
/// JNI callback from Kotlin when codec detection completes.
@@ -1170,6 +1231,9 @@ pub extern "system" fn Java_com_dtourolle_jellytau_player_JellyTauPlayer_00024Co
audio_codecs: JString,
max_audio_channels: jint,
) {
jni_guard(
"Java_com_dtourolle_jellytau_player_JellyTauPlayer_00024Companion_nativeOnCodecsDetected",
|| {
let video_str: String = env
.get_string(&video_codecs)
.map(|s| s.into())
@@ -1204,6 +1268,8 @@ pub extern "system" fn Java_com_dtourolle_jellytau_player_JellyTauPlayer_00024Co
if DETECTED_CODECS.set(codecs).is_err() {
log::error!("[CodecDetection] Failed to store codecs - already initialized");
}
},
);
}
/// Start the JellyTauPlaybackService if not already running.
+122
View File
@@ -0,0 +1,122 @@
//! Panic containment for the Android JNI boundary.
//!
//! Compiled on every platform, unlike `player::android` itself, so the guard and
//! the tripwire that enforces its use are unit-tested on the host — the same
//! reason `RESUME_BACKOFF_STEP_SECS` lives outside the `cfg(android)` block.
/// Run the body of a JNI callback with any panic contained.
///
/// Every `extern "system"` function in this file is called by the JVM on an
/// arbitrary thread. A panic that unwinds out of one crosses the FFI boundary,
/// which Rust answers by **aborting the process** — the app vanishes with no
/// Java exception, no stack trace attributable to it, and no crash report the
/// user can send. That is the worst possible failure mode for the callbacks
/// that fire four times a second during playback.
///
/// The panics are real, not theoretical: this file builds a fallback Tokio
/// runtime on threads that have none, and `Runtime::new()` fails under the fd
/// exhaustion and thread-spawn refusal an Android device puts a media app
/// through. Losing one position report is recoverable; losing the process is
/// not.
///
/// A contained panic still leaves whatever it interrupted half-done, so this is
/// a backstop, not a licence to panic. `utils::lock` already keeps a poisoned
/// mutex from cascading; this keeps the FFI boundary from turning any remaining
/// panic into a process kill.
///
/// TRACES: UR-005 | DR-052
///
/// Only *called* from `player::android`, which is `cfg(target_os = "android")`,
/// so it is dead code on every other target — the same reason
/// `RESUME_BACKOFF_STEP_SECS` carries this attribute. It is still compiled and
/// tested here on purpose.
#[cfg_attr(not(target_os = "android"), allow(dead_code))]
pub(crate) fn jni_guard<F: FnOnce()>(name: &str, body: F) {
// AssertUnwindSafe: the shared state behind these callbacks is already
// reached through poison-tolerant locks, so a panic cannot hand out a
// guard observing a torn value.
if std::panic::catch_unwind(std::panic::AssertUnwindSafe(body)).is_err() {
// The panic hook has already logged the payload and location.
log::error!("[JNI] Panic in {name} was contained; the callback was dropped");
}
}
// TRACES: UR-005 | DR-052 | UT-052
#[cfg(test)]
mod jni_guard_tests {
use super::*;
/// The guard must swallow a panic rather than let it reach the JVM.
///
/// TRACES: UR-005 | DR-052 | UT-052
#[test]
fn a_panicking_callback_body_does_not_escape_the_guard() {
let hook = std::panic::take_hook();
std::panic::set_hook(Box::new(|_| {}));
jni_guard("test_callback", || panic!("ExoPlayer callback blew up"));
std::panic::set_hook(hook);
// Reaching here at all is the assertion: without the guard the panic
// would unwind out of the `extern "system"` fn and abort the process.
}
/// The guard must not disturb a callback that behaves.
///
/// TRACES: UR-005 | DR-052 | UT-052
#[test]
fn a_normal_callback_body_still_runs() {
let mut ran = false;
jni_guard("test_callback", || ran = true);
assert!(ran);
}
/// **Tripwire.** Every JNI entry point must wrap its body in `jni_guard`.
///
/// A panic crossing the `extern "system"` boundary aborts the process, so a
/// twelfth callback added without the guard reintroduces the whole defect.
/// Checked against the source because the real boundary needs a JVM to
/// exercise — the same tripwire idiom as `check:boundary`.
///
/// TRACES: UR-005 | DR-052 | UT-052
#[test]
fn every_jni_entry_point_wraps_its_body_in_the_guard() {
let src = include_str!("android/mod.rs");
let mut unguarded = Vec::new();
let mut lines = src.lines().enumerate().peekable();
while let Some((_, line)) = lines.next() {
if !line.starts_with("pub extern \"system\" fn ") {
continue;
}
let name = line
.trim_start_matches("pub extern \"system\" fn ")
.trim_end_matches('(')
.to_string();
// Walk to the end of the parameter list, then look at the first
// statement of the body.
let mut body_start = None;
for (n, l) in lines.by_ref() {
if l.trim_end().ends_with(") {") || l.trim() == ") {" {
body_start = Some(n);
break;
}
}
assert!(body_start.is_some(), "could not find the body of {name}");
match lines.peek() {
Some((_, first)) if first.trim_start().starts_with("jni_guard(") => {}
other => unguarded.push(format!(
"{name} (body starts with {:?})",
other.map(|(_, l)| l.trim()).unwrap_or("<eof>")
)),
}
}
assert!(
unguarded.is_empty(),
"JNI entry points whose body is not wrapped in jni_guard — a panic in \
one of these aborts the process:\n {}",
unguarded.join("\n ")
);
}
}
+268
View File
@@ -0,0 +1,268 @@
//! A declared lock hierarchy for [`PlayerController`], and a tripwire that
//! enforces it.
//!
//! The controller carries seventeen separate mutexes, reached from the MPV event
//! loop, JNI callbacks, sleep/autoplay timers, the session poller and every IPC
//! command. Nothing about that arrangement prevents two threads taking the same
//! two locks in opposite orders, which deadlocks the player outright — and this
//! subsystem has already produced one deadlock (a tokio `MutexGuard` held in a
//! `match` scrutinee, which stalled the `AdvanceToNext` arm).
//!
//! Today the code is disciplined: acquisitions are scoped, and `previous()` for
//! instance explicitly drops the backend guard before touching the queue. But
//! that holds by convention, and convention is not checked. [`LOCK_ORDER`]
//! writes the convention down and `every_overlapping_acquisition_respects_the_order`
//! fails the build when a change breaks it.
//!
//! Ordering only matters where one guard is **still held** while another lock is
//! taken. Acquiring two locks one after another, each released before the next,
//! cannot deadlock — so the analysis looks for overlap, not for mere sequence.
//!
//! TRACES: UR-005 | DR-052
// This module is a static analysis of `player/mod.rs` plus the hierarchy it
// checks against. Its only caller is its own test module, but `LOCK_ORDER` is
// the documentation of record for how these locks nest, so it stays compiled
// (and rustdoc'd) rather than hidden behind `cfg(test)`.
#![allow(dead_code)]
/// The order in which `PlayerController`'s locks may be nested.
///
/// A thread already holding one of these may only acquire a lock that appears
/// **later** in this list. The order is not arbitrary — it follows the nesting
/// the code already relies on:
///
/// - `repository` and `sleep_timer` are taken by long-running decisions that go
/// on to consult playback state, so they sit outermost.
/// - `backend` outranks `queue`: "what is playing" is read before "what is
/// next", never the reverse.
/// - `event_emitter` is last. Emitting is a leaf — notifying the frontend must
/// never reach back for more player state.
///
/// TRACES: UR-005 | DR-052
pub const LOCK_ORDER: &[&str] = &[
"repository",
"sleep_timer",
"countdown_cancel",
"jellyfin_client",
"backend",
"queue",
"stream_resume",
"end_reason",
"reported_time",
"background_audio_active",
"background_audio_base",
"html5_playing",
"autoplay_settings",
"autoplay_episode_count",
"reports",
"event_emitter",
];
/// Rank of `field` in [`LOCK_ORDER`], or `None` if it is not a declared lock.
pub fn rank(field: &str) -> Option<usize> {
LOCK_ORDER.iter().position(|f| *f == field)
}
/// One lock acquired while another is still held.
#[derive(Debug, PartialEq, Eq)]
pub struct Overlap {
/// The lock already held.
pub outer: String,
/// The lock acquired underneath it.
pub inner: String,
/// 1-indexed line of the inner acquisition, for a useful failure message.
pub line: usize,
}
/// Find every place `src` takes a lock while holding another.
///
/// Deliberately simple and line-based: it tracks `let … = self.FIELD.lock_safe()`
/// bindings and looks for a different `self.OTHER.lock_safe()` before the
/// binding goes out of scope or is explicitly dropped. A guard that is not bound
/// to a name (`*self.flag.lock_safe() = false;`) is released at the end of its
/// statement and cannot overlap anything, so it is only ever an *inner*
/// acquisition here.
///
/// TRACES: UR-005 | DR-052 | UT-052
pub fn overlapping_acquisitions(src: &str) -> Vec<Overlap> {
let lines: Vec<&str> = src.lines().collect();
let mut found = Vec::new();
for (i, line) in lines.iter().enumerate() {
let Some((var, field)) = parse_binding(line) else {
continue;
};
let indent = line.len() - line.trim_start().len();
for (j, later) in lines.iter().enumerate().skip(i + 1) {
if later.contains(&format!("drop({var})")) {
break;
}
let trimmed = later.trim();
if trimmed.is_empty() {
continue;
}
// Left the block the guard lives in.
let later_indent = later.len() - later.trim_start().len();
if later_indent < indent && !trimmed.starts_with(['.', ')', '}']) {
break;
}
if trimmed == "}" && later_indent < indent {
break;
}
if let Some(inner) = parse_acquisition(later, field) {
found.push(Overlap {
outer: field.to_string(),
inner,
line: j + 1,
});
break;
}
}
}
found
}
/// `let [mut] name = self.field.lock_safe()` → `(name, field)`.
fn parse_binding(line: &str) -> Option<(&str, &str)> {
let rest = line.trim_start().strip_prefix("let ")?;
let rest = rest.strip_prefix("mut ").unwrap_or(rest);
let (name, rest) = rest.split_once(" = self.")?;
let (field, _) = rest.split_once(".lock_safe()")?;
if name.contains(' ') || field.contains('.') {
return None;
}
Some((name, field))
}
/// The first `self.other.lock_safe()` on `line` that is not `held`.
fn parse_acquisition(line: &str, held: &str) -> Option<String> {
let mut search = line;
while let Some(at) = search.find("self.") {
let after = &search[at + 5..];
if let Some((field, _)) = after.split_once(".lock_safe()") {
if !field.contains(['.', '(', ' ']) && field != held {
return Some(field.to_string());
}
}
search = after;
}
None
}
// TRACES: UR-005 | DR-052 | UT-052
#[cfg(test)]
mod tests {
use super::*;
/// **The tripwire.** Every nested acquisition in the controller must follow
/// [`LOCK_ORDER`].
///
/// A violation is a lock-order inversion: two threads taking the same pair
/// in opposite orders deadlock the player, and the symptom is a frozen app
/// with no error anywhere.
///
/// TRACES: UR-005 | DR-052 | UT-052
#[test]
fn every_overlapping_acquisition_respects_the_order() {
let src = include_str!("mod.rs");
let mut violations = Vec::new();
for overlap in overlapping_acquisitions(src) {
let (Some(outer), Some(inner)) = (rank(&overlap.outer), rank(&overlap.inner)) else {
violations.push(format!(
"player/mod.rs:{} takes '{}' while holding '{}', and one of them \
is not declared in LOCK_ORDER",
overlap.line, overlap.inner, overlap.outer
));
continue;
};
if outer >= inner {
violations.push(format!(
"player/mod.rs:{} takes '{}' (rank {inner}) while holding '{}' \
(rank {outer}) an inversion against LOCK_ORDER",
overlap.line, overlap.inner, overlap.outer
));
}
}
assert!(
violations.is_empty(),
"lock-order inversions in PlayerController:\n {}\n\nEither reorder the \
acquisitions or, if the new order is the correct one, change LOCK_ORDER \
and re-check every other site.",
violations.join("\n ")
);
}
/// The analysis must actually see the nesting the controller does today,
/// or the tripwire above passes by finding nothing.
///
/// TRACES: UR-005 | DR-052 | UT-052
#[test]
fn the_analysis_finds_the_nesting_that_exists() {
let found = overlapping_acquisitions(include_str!("mod.rs"));
assert!(
found.len() >= 5,
"expected the controller's known nested acquisitions, found {found:?}"
);
assert!(
found
.iter()
.any(|o| o.outer == "backend" && o.inner == "queue"),
"the backend->queue nesting in state() should be detected: {found:?}"
);
}
/// A guard held across a lock taken in the wrong order must be caught.
///
/// TRACES: UR-005 | DR-052 | UT-052
#[test]
fn an_inversion_is_detected() {
let src = " fn bad(&self) {\n\
\x20 let queue = self.queue.lock_safe();\n\
\x20 let b = self.backend.lock_safe();\n\
\x20 }\n";
let found = overlapping_acquisitions(src);
assert_eq!(found.len(), 1, "{found:?}");
assert_eq!(found[0].outer, "queue");
assert_eq!(found[0].inner, "backend");
assert!(rank("queue").unwrap() > rank("backend").unwrap());
}
/// Sequential, non-overlapping acquisitions cannot deadlock and must not be
/// reported — `previous()` drops the backend guard before taking the queue.
///
/// TRACES: UR-005 | DR-052 | UT-052
#[test]
fn a_dropped_guard_is_not_an_overlap() {
let src = " fn fine(&self) {\n\
\x20 let backend = self.backend.lock_safe();\n\
\x20 drop(backend);\n\
\x20 let queue = self.queue.lock_safe();\n\
\x20 }\n";
assert!(overlapping_acquisitions(src).is_empty());
}
/// Every declared lock name must be a real field, or the order documents
/// something that no longer exists.
///
/// TRACES: UR-005 | DR-052 | UT-052
#[test]
fn every_declared_lock_is_a_real_field() {
let src = include_str!("mod.rs");
let decl = src
.split_once("pub struct PlayerController {")
.expect("PlayerController struct")
.1;
let decl = decl.split_once("\n}").expect("end of struct").0;
for name in LOCK_ORDER {
assert!(
decl.contains(&format!("{name}:")),
"LOCK_ORDER names '{name}', which is not a PlayerController field"
);
}
}
}
+8
View File
@@ -28,6 +28,14 @@ pub mod track_switch;
#[cfg(test)]
mod mpv_backend_test;
// The declared lock hierarchy for `PlayerController` below, and the tripwire
// that enforces it. See the module docs for why seventeen mutexes need one.
pub mod lock_order;
// Panic containment for the JNI boundary. Not gated on the target: the guard
// and its tripwire test are exercised on the host, where `android` never builds.
pub mod jni_guard;
// Platform-specific backends
#[cfg(target_os = "android")]
pub mod android;
+215 -63
View File
@@ -18,6 +18,38 @@ use tokio::time::{timeout, Duration};
use super::exclusions::ExcludeHidden;
use super::{types::*, MediaRepository, OfflineRepository, OnlineRepository};
/// The cache side of a cache-first query.
///
/// Either the cache answered inside the fast path, or it is still working and
/// the query can be collected later. Keeping the slow case *addressable* rather
/// than discarding it is what lets an offline query fall back to cached content
/// after the server leg fails.
///
/// TRACES: UR-002 | DR-013
enum CacheLeg<T> {
/// The cache answered within [`HybridRepository::CACHE_FAST_PATH`].
Ready(Result<T, RepoError>),
/// Still running. Awaiting the handle yields the answer eventually.
Slow(tokio::task::JoinHandle<Result<T, RepoError>>),
}
impl<T> CacheLeg<T> {
/// Split into the fast-path answer and the still-running query. Exactly one
/// side is `Some`.
#[allow(clippy::type_complexity)]
fn split(
self,
) -> (
Option<Result<T, RepoError>>,
Option<tokio::task::JoinHandle<Result<T, RepoError>>>,
) {
match self {
CacheLeg::Ready(result) => (Some(result), None),
CacheLeg::Slow(handle) => (None, Some(handle)),
}
}
}
/// Hybrid repository combining online and offline data sources
///
/// Uses cache-first parallel racing strategy:
@@ -379,35 +411,12 @@ impl HybridRepository {
/// @req: DR-013 - Repository pattern for online/offline data access
///
/// TRACES: UR-002, UR-076 | DR-013, DR-209
async fn parallel_race<T, F1, F2>(
&self,
cache_future: F1,
server_future: F2,
) -> Result<T, RepoError>
async fn parallel_race<T, F2>(cache: CacheLeg<T>, server_future: F2) -> Result<T, RepoError>
where
T: MeaningfulContent + ExcludeHidden + Clone + Send + 'static,
F1: std::future::Future<Output = Result<T, RepoError>> + Send,
F2: std::future::Future<Output = Result<T, RepoError>> + Send,
{
// Try cache first (100ms timeout already applied by callers)
let cache_result = cache_future.await.map(ExcludeHidden::without_excluded);
if let Ok(data) = &cache_result {
if data.has_content() {
debug!("[HybridRepo] Cache hit, returning immediately");
return Ok(data.clone());
}
}
// Cache miss — fall back to server
debug!("[HybridRepo] Cache miss, querying server");
match server_future.await {
Ok(data) => Ok(data.without_excluded()),
Err(e) => {
// Server failed, try to return cache even if empty
cache_result.or(Err(e))
}
}
Self::race_with_refresh(cache, server_future, || {}).await
}
/// [`Self::parallel_race`], plus a callback fired on the fast path so the
@@ -424,21 +433,20 @@ impl HybridRepository {
/// already being fetched and cached by the normal path.
///
/// TRACES: UR-002, UR-025, UR-076 | DR-155, DR-209
async fn race_with_refresh<T, F1, F2, R>(
&self,
cache_future: F1,
async fn race_with_refresh<T, F2, R>(
cache: CacheLeg<T>,
server_future: F2,
on_cache_hit: R,
) -> Result<T, RepoError>
where
T: MeaningfulContent + ExcludeHidden + Clone + Send + 'static,
F1: std::future::Future<Output = Result<T, RepoError>> + Send,
F2: std::future::Future<Output = Result<T, RepoError>> + Send,
R: FnOnce(),
{
let cache_result = cache_future.await.map(ExcludeHidden::without_excluded);
let (fast, slow) = cache.split();
let fast = fast.map(|r| r.map(ExcludeHidden::without_excluded));
if let Ok(data) = &cache_result {
if let Some(Ok(data)) = &fast {
if data.has_content() {
debug!("[HybridRepo] Cache hit, returning immediately (refreshing in background)");
on_cache_hit();
@@ -446,21 +454,81 @@ impl HybridRepository {
}
}
debug!("[HybridRepo] Cache miss, querying server");
debug!("[HybridRepo] Cache miss or slow, querying server");
match server_future.await {
Ok(data) => Ok(data.without_excluded()),
Err(e) => cache_result.or(Err(e)),
Err(e) => {
// The server cannot answer. If the cache is still working, it is
// now the only thing that can, so wait it out rather than
// reporting the server's failure over data we are about to hold.
// This is the offline path: a cache read slowed by a concurrent
// write used to surface as a network error.
if let Some(handle) = slow {
debug!("[HybridRepo] Server failed; waiting for the slow cache query");
return match handle.await {
Ok(Ok(data)) => Ok(data.without_excluded()),
Ok(Err(cache_err)) => {
debug!("[HybridRepo] Slow cache query also failed: {cache_err}");
Err(e)
}
Err(join) => {
debug!("[HybridRepo] Slow cache query panicked: {join}");
Err(e)
}
};
}
// Cache answered in time but had nothing: return that, so an
// empty-but-valid cached listing still beats a network error.
fast.unwrap_or(Err(e))
}
}
}
/// Simple timeout wrapper for cache queries (100ms timeout)
/// How long the cache gets to answer before a query falls through to the
/// server. Short on purpose: this bounds how long a *cache hit* may delay
/// the UI, not how long the query is allowed to take.
const CACHE_FAST_PATH: Duration = Duration::from_millis(100);
/// Start a cache query and give it [`Self::CACHE_FAST_PATH`] to answer.
///
/// @req: DR-013 - Repository pattern (cache-first with timeout)
/// Missing the deadline does **not** cancel the query — it keeps running on
/// its own task and [`CacheLeg::settle`] can still collect it. That
/// distinction is the whole point. The database is one SQLite connection
/// behind one mutex, so a concurrent write (a sync drain, a bulk
/// `save_to_cache`) blocks reads for its duration and this deadline trips
/// routinely on slow storage. Treating that as "the cache is empty" while
/// throwing the answer away meant that offline — where the server leg also
/// fails — browsing surfaced a network error instead of the cached content
/// sitting right there on disk.
///
/// TRACES: UR-002 | DR-013
async fn cache_leg<T>(
future: impl std::future::Future<Output = Result<T, RepoError>> + Send + 'static,
) -> CacheLeg<T>
where
T: Send + 'static,
{
// `&mut handle` so the timeout borrows the join handle rather than
// consuming it: on expiry the task is still ours to collect.
let mut handle = tokio::spawn(future);
match timeout(Self::CACHE_FAST_PATH, &mut handle).await {
Ok(Ok(result)) => CacheLeg::Ready(result),
Ok(Err(join)) => CacheLeg::Ready(Err(RepoError::Database {
message: format!("Cache query failed: {join}"),
})),
Err(_) => {
debug!("[HybridRepo] Cache missed the fast path; leaving it running");
CacheLeg::Slow(handle)
}
}
}
/// Await a cache query that is still running, however long it takes.
async fn cache_with_timeout<T>(
&self,
future: impl std::future::Future<Output = Result<T, RepoError>> + Send,
) -> Result<T, RepoError> {
timeout(Duration::from_millis(100), future)
timeout(Self::CACHE_FAST_PATH, future)
.await
.unwrap_or_else(|_| {
Err(RepoError::Database {
@@ -657,7 +725,7 @@ impl MediaRepository for HybridRepository {
let item_id = item_id.to_string();
let item_id_clone = item_id.clone();
let cache_future = self.cache_with_timeout(async move { offline.get_item(&item_id).await });
let cache_future = Self::cache_leg(async move { offline.get_item(&item_id).await }).await;
let online_for_refresh = Arc::clone(&self.online);
let offline_for_save = Arc::clone(&self.offline);
@@ -683,8 +751,7 @@ impl MediaRepository for HybridRepository {
let server_future = async move { online.get_item(&item_id_clone).await };
self.race_with_refresh(cache_future, server_future, on_cache_hit)
.await
Self::race_with_refresh(cache_future, server_future, on_cache_hit).await
}
async fn get_latest_items(
@@ -698,13 +765,13 @@ impl MediaRepository for HybridRepository {
let parent_id_clone = parent_id.clone();
let limit_clone = limit;
let cache_future = self
.cache_with_timeout(async move { offline.get_latest_items(&parent_id, limit).await });
let cache_future =
Self::cache_leg(async move { offline.get_latest_items(&parent_id, limit).await }).await;
let server_future =
async move { online.get_latest_items(&parent_id_clone, limit_clone).await };
self.parallel_race(cache_future, server_future).await
Self::parallel_race(cache_future, server_future).await
}
async fn get_resume_items(
@@ -718,11 +785,12 @@ impl MediaRepository for HybridRepository {
let parent_id_clone = parent_id_str.clone();
let limit_clone = limit;
let cache_future = self.cache_with_timeout(async move {
let cache_future = Self::cache_leg(async move {
offline
.get_resume_items(parent_id_str.as_deref(), limit)
.await
});
})
.await;
let server_future = async move {
online
@@ -730,7 +798,7 @@ impl MediaRepository for HybridRepository {
.await
};
self.parallel_race(cache_future, server_future).await
Self::parallel_race(cache_future, server_future).await
}
async fn get_next_up_episodes(
@@ -754,11 +822,11 @@ impl MediaRepository for HybridRepository {
let limit_clone = limit;
let cache_future =
self.cache_with_timeout(async move { offline.get_recently_played_audio(limit).await });
Self::cache_leg(async move { offline.get_recently_played_audio(limit).await }).await;
let server_future = async move { online.get_recently_played_audio(limit_clone).await };
self.parallel_race(cache_future, server_future).await
Self::parallel_race(cache_future, server_future).await
}
async fn get_resume_movies(&self, limit: Option<usize>) -> Result<Vec<MediaItem>, RepoError> {
@@ -767,11 +835,11 @@ impl MediaRepository for HybridRepository {
let limit_clone = limit;
let cache_future =
self.cache_with_timeout(async move { offline.get_resume_movies(limit).await });
Self::cache_leg(async move { offline.get_resume_movies(limit).await }).await;
let server_future = async move { online.get_resume_movies(limit_clone).await };
self.parallel_race(cache_future, server_future).await
Self::parallel_race(cache_future, server_future).await
}
async fn get_rediscover_albums(
@@ -784,11 +852,12 @@ impl MediaRepository for HybridRepository {
let parent_id_owned = parent_id.map(|s| s.to_string());
let parent_id_clone = parent_id_owned.clone();
let cache_future = self.cache_with_timeout(async move {
let cache_future = Self::cache_leg(async move {
offline
.get_rediscover_albums(parent_id_owned.as_deref(), limit)
.await
});
})
.await;
let server_future = async move {
online
@@ -796,7 +865,7 @@ impl MediaRepository for HybridRepository {
.await
};
self.parallel_race(cache_future, server_future).await
Self::parallel_race(cache_future, server_future).await
}
async fn get_genres(&self, parent_id: Option<&str>) -> Result<Vec<Genre>, RepoError> {
@@ -869,11 +938,11 @@ impl MediaRepository for HybridRepository {
let opts_clone = options.clone();
let cache_future =
self.cache_with_timeout(async move { offline.search(&query, opts_clone).await });
Self::cache_leg(async move { offline.search(&query, opts_clone).await }).await;
let server_future = async move { online.search(&query_clone, options).await };
self.parallel_race(cache_future, server_future).await
Self::parallel_race(cache_future, server_future).await
}
async fn get_playback_info(&self, item_id: &str) -> Result<PlaybackInfo, RepoError> {
@@ -1019,11 +1088,11 @@ impl MediaRepository for HybridRepository {
let person_id_clone = person_id.clone();
let cache_future =
self.cache_with_timeout(async move { offline.get_person(&person_id).await });
Self::cache_leg(async move { offline.get_person(&person_id).await }).await;
let server_future = async move { online.get_person(&person_id_clone).await };
self.parallel_race(cache_future, server_future).await
Self::parallel_race(cache_future, server_future).await
}
async fn get_items_by_person(
@@ -1037,14 +1106,16 @@ impl MediaRepository for HybridRepository {
let person_id_clone = person_id.clone();
let opts_clone = options.clone();
let cache_future = self.cache_with_timeout(async move {
offline.get_items_by_person(&person_id, opts_clone).await
});
let cache_future =
Self::cache_leg(
async move { offline.get_items_by_person(&person_id, opts_clone).await },
)
.await;
let server_future =
async move { online.get_items_by_person(&person_id_clone, options).await };
self.parallel_race(cache_future, server_future).await
Self::parallel_race(cache_future, server_future).await
}
/// TRACES: UR-067 | DR-115
@@ -1092,12 +1163,12 @@ impl MediaRepository for HybridRepository {
let item_id = item_id.to_string();
let item_id_clone = item_id.clone();
let cache_future = self
.cache_with_timeout(async move { offline.get_similar_items(&item_id, limit).await });
let cache_future =
Self::cache_leg(async move { offline.get_similar_items(&item_id, limit).await }).await;
let server_future = async move { online.get_similar_items(&item_id_clone, limit).await };
self.parallel_race(cache_future, server_future).await
Self::parallel_race(cache_future, server_future).await
}
// ===== Playlist Methods =====
@@ -1228,6 +1299,87 @@ mod tests {
use super::*;
use std::sync::Mutex;
/// Offline, a cache read slowed past the fast path must still answer.
///
/// The database is one SQLite connection behind one mutex, so a concurrent
/// write blocks reads for its duration and the 100 ms fast path trips on
/// slow storage. The deadline used to *cancel* the read and report it as a
/// miss; with the server leg also failing (offline), the user got a network
/// error over cached content that was sitting on disk.
///
/// TRACES: UR-002 | DR-013
#[tokio::test]
async fn a_slow_cache_still_answers_when_the_server_is_gone() {
let cache = HybridRepository::cache_leg(async {
tokio::time::sleep(Duration::from_millis(250)).await;
Ok(vec![MediaItem {
id: "cached-item".to_string(),
..Default::default()
}])
})
.await;
let server = async {
Err(RepoError::Network {
message: "offline".to_string(),
})
};
let got = HybridRepository::parallel_race(cache, server)
.await
.expect("a slow cache read must still be delivered when the server is gone");
assert_eq!(got.len(), 1);
assert_eq!(got[0].id, "cached-item");
}
/// A cache that beats the deadline still short-circuits the server.
///
/// TRACES: UR-002 | DR-013
#[tokio::test]
async fn a_fast_cache_hit_never_reaches_the_server() {
let cache = HybridRepository::cache_leg(async {
Ok(vec![MediaItem {
id: "fast".to_string(),
..Default::default()
}])
})
.await;
let server = async {
panic!("the server leg must not run on a cache hit");
};
let got = HybridRepository::parallel_race(cache, server)
.await
.unwrap();
assert_eq!(got[0].id, "fast");
}
/// When both sides fail, the server's error is what the caller sees.
///
/// TRACES: UR-002 | DR-013
#[tokio::test]
async fn a_failing_slow_cache_reports_the_server_error() {
let cache: CacheLeg<Vec<MediaItem>> = HybridRepository::cache_leg(async {
tokio::time::sleep(Duration::from_millis(250)).await;
Err(RepoError::Database {
message: "disk gone".to_string(),
})
})
.await;
let server = async {
Err(RepoError::Network {
message: "offline".to_string(),
})
};
let err = HybridRepository::parallel_race(cache, server)
.await
.unwrap_err();
assert!(matches!(err, RepoError::Network { .. }), "got {err:?}");
}
/// Mock offline repository that tracks queries and saves
struct MockOfflineRepo {
items: Arc<Mutex<Vec<MediaItem>>>,
+373 -11
View File
@@ -84,6 +84,30 @@ fn build_fts_prefix_query(query: &str) -> Option<String> {
)
}
/// The Jellyfin taxonomy half of "does cached item `i` belong to library `l`":
/// the library's `collection_type` against the item's `item_type`.
///
/// A macro rather than a `const` because both callers need it *inside* a larger
/// SQL string literal, and `concat!` cannot take a const. One definition, so the
/// two sites cannot drift — they did once already, and opening any downloaded
/// library then listed every downloaded item on the server (DR-167).
///
/// Deliberately has **no fall-open arm**. Adding "…or the type is unknown"
/// makes the clause true for every row, which is precisely the defect it exists
/// to prevent; callers that want that behaviour must say so themselves and
/// justify it, as `LIBRARY_HOLDS_ITEM` does.
///
/// TRACES: UR-007, UR-055 | DR-167, DR-277
macro_rules! library_type_matches_item {
() => {
"(
(l.collection_type = 'music' AND i.item_type IN ('MusicAlbum', 'MusicArtist', 'Audio'))
OR (l.collection_type = 'movies' AND i.item_type = 'Movie')
OR (l.collection_type = 'tvshows' AND i.item_type IN ('Series', 'Season', 'Episode'))
)"
};
}
pub struct OfflineRepository {
db_service: Arc<RusqliteService>,
server_id: String,
@@ -422,12 +446,81 @@ impl OfflineRepository {
result
}
/// Which library the children of `parent_id` belong to.
///
/// `Some(parent_id)` when the parent is itself a library, otherwise the
/// library the parent item was already filed under — so the association
/// propagates down a hierarchy as it is browsed, without needing the server
/// to repeat it on every item. `None` for a parent that is neither, which
/// is how synthetic parents like "favorites" avoid being filed anywhere.
///
/// TRACES: UR-007 | DR-278
async fn resolve_owning_library(&self, parent_id: &str) -> Option<String> {
let is_library: Option<String> = self
.db_service
.query_optional(
Query::with_params(
"SELECT id FROM libraries WHERE id = ? AND server_id = ?",
vec![
QueryParam::String(parent_id.to_string()),
QueryParam::String(self.server_id.clone()),
],
),
|row| row.get(0),
)
.await
.ok()
.flatten();
if is_library.is_some() {
return is_library;
}
self.db_service
.query_optional(
Query::with_params(
"SELECT library_id FROM items WHERE id = ? AND library_id IS NOT NULL",
vec![QueryParam::String(parent_id.to_string())],
),
|row| row.get(0),
)
.await
.ok()
.flatten()
}
async fn save_to_cache_impl(
&self,
parent_id: &str,
items: &[MediaItem],
now: &str,
) -> Result<usize, RepoError> {
// Which library do these items belong to?
//
// Resolved once per call, from the parent being browsed. Two cases and
// nothing else:
//
// * the parent IS a library -> these are its direct children
// * the parent is an item -> inherit whatever library that item is
// already known to belong to, so tracks
// under an album and episodes under a
// season land in the same library as
// their container
//
// Synthetic parents ("favorites" and friends) match neither and stay
// NULL, which is correct: they are not a library and their contents
// span several.
//
// Until this existed, `library_id` was bound NULL for every cached row
// and the only way to associate an item with a library was the
// `collection_type` ↔ `item_type` taxonomy. That cannot tell two
// libraries of the *same* type apart — a server with "TV" and "Shows"
// served both the same contents — and has nothing to say about a
// library whose type it does not map (DR-278).
//
// TRACES: UR-007 | DR-278
let owning_library = self.resolve_owning_library(parent_id).await;
// Collect all unique parent IDs referenced by items being saved
let mut parent_ids = std::collections::HashSet::new();
parent_ids.insert(parent_id.to_string());
@@ -554,8 +647,12 @@ impl OfflineRepository {
vec![
QueryParam::String(item.id.clone()),
QueryParam::String(self.server_id.clone()),
// Library is NULL for cached items (may not be synced yet)
QueryParam::Null, // library_id
// The library this browse belongs to; NULL only for
// synthetic parents. See `resolve_owning_library`.
match &owning_library {
Some(lib) => QueryParam::String(lib.clone()),
None => QueryParam::Null,
}, // library_id
// Use the item's actual parent_id, not the function parameter
match &item.parent_id {
Some(pid) => QueryParam::String(pid.clone()),
@@ -856,13 +953,13 @@ impl OfflineRepository {
/// no mapping to narrow it by and hiding its contents would be worse.
///
/// TRACES: UR-055 | DR-082, DR-167
const LIBRARY_HOLDS_ITEM: &'static str = "(
(l.collection_type = 'music' AND i.item_type IN ('MusicAlbum', 'MusicArtist', 'Audio'))
OR (l.collection_type = 'movies' AND i.item_type = 'Movie')
OR (l.collection_type = 'tvshows' AND i.item_type IN ('Series', 'Season', 'Episode'))
OR l.collection_type IS NULL
const LIBRARY_HOLDS_ITEM: &'static str = concat!(
"(",
library_type_matches_item!(),
" OR l.collection_type IS NULL
OR l.collection_type NOT IN ('music', 'movies', 'tvshows')
)";
)"
);
/// TRACES: UR-055 | DR-082, DR-083
const DOWNLOADED_ITEMS_CTE: &'static str = "
@@ -1358,14 +1455,43 @@ impl MediaRepository for OfflineRepository {
-- so match every item on the server and let the type filter
-- (e.g. MusicAlbum / Movie / Series) narrow it. This is what
-- makes library landing pages show albums/movies/shows offline.
--
-- The type correlation is NOT optional. Without it this
-- EXISTS never mentions the item, so it is true for every
-- cached row as soon as the requested parent is any library.
-- Music/Movies/TV got away with that because their landing
-- pages pass `include_item_types`, which narrowed the result;
-- the generic library page passes none, so a Books or Photos
-- library served the entire cached server (DR-277).
--
-- `library_id` wins wherever it survived the cache write:
-- it is the server's own answer, and it is the only thing
-- that can scope a library whose type has no mapping (Books,
-- Photos, Collections) or none at all (a mixed library, where
-- Jellyfin sends CollectionType null). The taxonomy is the
-- fallback for rows that predate it being stored.
--
-- A library with neither a stored link nor a mapped type now
-- matches nothing here and falls through to the server, which
-- does know what is in it. Showing nothing briefly beats
-- showing somebody else's films with confidence.
OR EXISTS (
SELECT 1 FROM libraries l
WHERE l.id = ? AND l.server_id = i.server_id
AND (
i.library_id = l.id
OR (i.library_id IS NULL AND {})
)
)
){}{}
ORDER BY {}
LIMIT {} OFFSET {}",
type_filter, favorites_filter, order_by, limit, start_index
library_type_matches_item!(),
type_filter,
favorites_filter,
order_by,
limit,
start_index
);
// The requested id is compared against every hierarchy-linkage column
@@ -4587,6 +4713,27 @@ mod tests {
let db_service = create_test_db();
seed_favorites(&db_service).await;
// A favourite album *in lib-1*. The fixture's `album-fav` lives in
// lib-2, and this test used to expect it back from a lib-1 listing —
// which only held because the library clause matched every cached row
// regardless of which library it was in (DR-277). The assertions below
// still span both requested types, which is what UT-206 is really about;
// they now do it with an album that is actually in the library.
db_service
.execute(Query::new(
"INSERT INTO items (id, server_id, name, item_type, library_id, synced_at, sort_name) \
VALUES ('album-lib1', 'test-server', 'Album In Lib One', 'MusicAlbum', 'lib-1', '2026-01-01', 'Album In Lib One')",
))
.await
.unwrap();
db_service
.execute(Query::new(
"INSERT INTO user_data (user_id, item_id, is_favorite) VALUES ('test-user', 'album-lib1', 1)",
))
.await
.unwrap();
let repo = OfflineRepository::new(
db_service,
"test-server".to_string(),
@@ -4605,7 +4752,11 @@ mod tests {
.unwrap();
let mut ids: Vec<&str> = both.items.iter().map(|i| i.id.as_str()).collect();
ids.sort();
assert_eq!(ids, vec!["album-fav", "movie-fav", "movie-plain"]);
assert_eq!(ids, vec!["album-lib1", "movie-fav", "movie-plain"]);
assert!(
!ids.contains(&"album-fav"),
"album-fav belongs to lib-2 and must not appear in a lib-1 listing"
);
// Two type placeholders *and* the favourites parameter after them.
let favourites = repo
@@ -4621,7 +4772,7 @@ mod tests {
.unwrap();
let mut ids: Vec<&str> = favourites.items.iter().map(|i| i.id.as_str()).collect();
ids.sort();
assert_eq!(ids, vec!["album-fav", "movie-fav"]);
assert_eq!(ids, vec!["album-lib1", "movie-fav"]);
}
/// UT-102 — caching a server result mirrors its favourite state locally,
@@ -4937,4 +5088,215 @@ mod tests {
"a position with no favourite flag must still be mirrored"
);
}
/// A library whose `collection_type` is not one of the three the app has
/// landing pages for — Books, Photos, Home Videos, Collections, or a mixed
/// library — must not show the entire server.
///
/// The cache has no item→library link at all (`library_id`/`parent_id` are
/// NULL, see [[offline-libraries-never-cached]]), so `get_items` matched a
/// library parent with an EXISTS that never referenced the item:
///
/// OR EXISTS (SELECT 1 FROM libraries l WHERE l.id = ? AND ...)
///
/// True for every cached row the moment the requested id is any library.
/// The music/movies/TV landing pages got away with it because each passes
/// `include_item_types`, which narrowed the result; the generic library page
/// passes none, so opening a Books library served whatever happened to be
/// cached — films, albums, episodes. Same defect the downloaded listing had
/// in DR-167, in the path nobody re-checked.
///
/// TRACES: UR-007, UR-055 | DR-277 | UT-247
#[tokio::test]
async fn test_get_items_unknown_library_type_does_not_return_whole_server() {
// Shared global: other tests flip it, so hold the lock and
// state it explicitly rather than inheriting whatever ran last.
let _guard = lock_catalog_browse();
set_include_catalog_browse(true);
let db = create_test_db();
insert_item(&db, "movie-1", "Movie", None, None, None).await;
insert_item(&db, "album-1", "MusicAlbum", None, None, None).await;
insert_item(&db, "series-1", "Series", None, None, None).await;
// Every library kind the app has no landing page for, including the
// empty `collection_type` Jellyfin sends for a mixed library.
for collection_type in ["books", "boxsets", "photos", "homevideos", ""] {
let lib = format!("lib-{collection_type}");
seed_library(&db, &lib, collection_type).await;
let repo = make_repo(&db);
let ids: Vec<String> = repo
.get_items(&lib, None)
.await
.unwrap()
.items
.iter()
.map(|i| i.id.clone())
.collect();
assert!(
ids.is_empty(),
"a '{collection_type}' library must not serve the server's films, \
albums and shows; got {:?}",
ids
);
}
}
/// Two libraries of the *same* type are still two libraries. A server with
/// "TV" and "Shows" — or "Films" and "Kids Films" — must not serve both the
/// same contents.
///
/// The taxonomy fallback cannot tell them apart: it matches on
/// `collection_type`, which is identical for both, so every Series on the
/// server satisfies either one. Only the stored `library_id` can separate
/// them, which is why populating it is the real fix rather than a nicety.
///
/// TRACES: UR-007 | DR-277 | UT-250
#[tokio::test]
async fn test_get_items_two_libraries_of_one_type_are_not_interchangeable() {
// Shared global: other tests flip it, so hold the lock and
// state it explicitly rather than inheriting whatever ran last.
let _guard = lock_catalog_browse();
set_include_catalog_browse(true);
let db = create_test_db();
seed_library(&db, "tv-lib", "tvshows").await;
seed_library(&db, "shows-lib", "tvshows").await;
let repo = make_repo(&db);
// Seeded through the real write path, because that is what the fix
// changes: browsing a library is what files its contents under it.
for (id, lib) in [("series-a", "tv-lib"), ("series-b", "shows-lib")] {
let mut item = create_test_item(id, id, None);
item.item_type = "Series".to_string();
item.kind = crate::domain::MediaKind::Series;
repo.save_to_cache(lib, &[item]).await.unwrap();
}
for (lib, own, other) in [
("tv-lib", "series-a", "series-b"),
("shows-lib", "series-b", "series-a"),
] {
let ids: Vec<String> = repo
.get_items(lib, None)
.await
.unwrap()
.items
.iter()
.map(|i| i.id.clone())
.collect();
assert!(
ids.contains(&own.to_string()),
"{lib} should list {own}; got {:?}",
ids
);
assert!(
!ids.contains(&other.to_string()),
"{lib} must not list {other}, which lives in the other library; got {:?}",
ids
);
}
}
/// Opening an individual collection is a different path and must keep
/// working: a BoxSet's children carry `parent_id`, which the cache does
/// store, so they are matched by the ordinary parent link rather than by
/// the library clause this fix narrowed.
///
/// Worth pinning separately — narrowing the library clause could plausibly
/// have taken collections with it, and "Collections is empty" would look
/// identical to the bug it was meant to fix.
///
/// TRACES: UR-007 | DR-277 | UT-249
#[tokio::test]
async fn test_get_items_collection_lists_its_own_children() {
// Shared global: other tests flip it, so hold the lock and
// state it explicitly rather than inheriting whatever ran last.
let _guard = lock_catalog_browse();
set_include_catalog_browse(true);
let db = create_test_db();
seed_library(&db, "boxset-lib", "boxsets").await;
insert_item(&db, "boxset-1", "BoxSet", None, None, None).await;
insert_item(&db, "outsider", "Movie", None, None, None).await;
// A film inside the collection: linked by parent_id, which is what a
// BoxSet's children actually carry.
db.execute(Query::with_params(
"INSERT INTO items (id, server_id, name, item_type, parent_id, synced_at) \
VALUES ('in-set', 'test-server', 'In The Set', 'Movie', ?1, '2024-01-01')",
vec![QueryParam::String("boxset-1".to_string())],
))
.await
.unwrap();
let repo = make_repo(&db);
let ids: Vec<String> = repo
.get_items("boxset-1", None)
.await
.unwrap()
.items
.iter()
.map(|i| i.id.clone())
.collect();
assert_eq!(
ids,
vec!["in-set".to_string()],
"a collection lists its own children and nothing else; got {:?}",
ids
);
}
/// The narrowing must not break the libraries that *do* have landing pages:
/// they reach the same query and must keep returning their own media.
///
/// TRACES: UR-007 | DR-277 | UT-248
#[tokio::test]
async fn test_get_items_typed_libraries_still_return_their_own_media() {
// Shared global: other tests flip it, so hold the lock and
// state it explicitly rather than inheriting whatever ran last.
let _guard = lock_catalog_browse();
set_include_catalog_browse(true);
let db = create_test_db();
seed_library(&db, "music-lib", "music").await;
seed_library(&db, "movie-lib", "movies").await;
seed_library(&db, "tv-lib", "tvshows").await;
insert_item(&db, "album-1", "MusicAlbum", None, None, None).await;
insert_item(&db, "movie-1", "Movie", None, None, None).await;
insert_item(&db, "series-1", "Series", None, None, None).await;
let repo = make_repo(&db);
for (lib, expected, forbidden) in [
("music-lib", "album-1", "movie-1"),
("movie-lib", "movie-1", "album-1"),
("tv-lib", "series-1", "album-1"),
] {
let ids: Vec<String> = repo
.get_items(lib, None)
.await
.unwrap()
.items
.iter()
.map(|i| i.id.clone())
.collect();
assert!(
ids.contains(&expected.to_string()),
"{lib} should list {expected}; got {:?}",
ids
);
assert!(
!ids.contains(&forbidden.to_string()),
"{lib} must not list {forbidden}; got {:?}",
ids
);
}
}
}
+73 -21
View File
@@ -7,6 +7,7 @@
//! - Test with different database backends
//! - Migrate to other database systems in the future
use crate::utils::lock::MutexSafe;
use async_trait::async_trait;
use rusqlite::{params_from_iter, Connection, Result as SqliteResult, Row};
use std::sync::{Arc, Mutex};
@@ -128,9 +129,10 @@ impl DatabaseService for RusqliteService {
async fn execute(&self, query: Query) -> DbResult<usize> {
let conn = Arc::clone(&self.conn);
tokio::task::spawn_blocking(move || {
let conn = conn
.lock()
.map_err(|e| format!("Failed to lock connection: {}", e))?;
// `lock_safe`, not `lock`: this is the busiest lock in the app and a
// panic under the guard would otherwise poison it, failing every
// later query with "poisoned lock" until the process restarts.
let conn = conn.lock_safe();
execute_query(&conn, query)
})
.await
@@ -141,9 +143,10 @@ impl DatabaseService for RusqliteService {
let conn = Arc::clone(&self.conn);
let sql = sql.to_string();
tokio::task::spawn_blocking(move || {
let conn = conn
.lock()
.map_err(|e| format!("Failed to lock connection: {}", e))?;
// `lock_safe`, not `lock`: this is the busiest lock in the app and a
// panic under the guard would otherwise poison it, failing every
// later query with "poisoned lock" until the process restarts.
let conn = conn.lock_safe();
conn.execute_batch(&sql)
.map_err(|e| format!("Execute batch failed: {}", e))
})
@@ -158,9 +161,10 @@ impl DatabaseService for RusqliteService {
{
let conn = Arc::clone(&self.conn);
tokio::task::spawn_blocking(move || {
let conn = conn
.lock()
.map_err(|e| format!("Failed to lock connection: {}", e))?;
// `lock_safe`, not `lock`: this is the busiest lock in the app and a
// panic under the guard would otherwise poison it, failing every
// later query with "poisoned lock" until the process restarts.
let conn = conn.lock_safe();
query_one(&conn, query, mapper)
})
.await
@@ -174,9 +178,10 @@ impl DatabaseService for RusqliteService {
{
let conn = Arc::clone(&self.conn);
tokio::task::spawn_blocking(move || {
let conn = conn
.lock()
.map_err(|e| format!("Failed to lock connection: {}", e))?;
// `lock_safe`, not `lock`: this is the busiest lock in the app and a
// panic under the guard would otherwise poison it, failing every
// later query with "poisoned lock" until the process restarts.
let conn = conn.lock_safe();
query_optional(&conn, query, mapper)
})
.await
@@ -190,9 +195,10 @@ impl DatabaseService for RusqliteService {
{
let conn = Arc::clone(&self.conn);
tokio::task::spawn_blocking(move || {
let conn = conn
.lock()
.map_err(|e| format!("Failed to lock connection: {}", e))?;
// `lock_safe`, not `lock`: this is the busiest lock in the app and a
// panic under the guard would otherwise poison it, failing every
// later query with "poisoned lock" until the process restarts.
let conn = conn.lock_safe();
query_many(&conn, query, mapper)
})
.await
@@ -206,9 +212,10 @@ impl DatabaseService for RusqliteService {
{
let conn = Arc::clone(&self.conn);
tokio::task::spawn_blocking(move || {
let conn = conn
.lock()
.map_err(|e| format!("Failed to lock connection: {}", e))?;
// `lock_safe`, not `lock`: this is the busiest lock in the app and a
// panic under the guard would otherwise poison it, failing every
// later query with "poisoned lock" until the process restarts.
let conn = conn.lock_safe();
conn.execute("BEGIN TRANSACTION", [])
.map_err(|e| format!("Failed to begin transaction: {}", e))?;
@@ -236,9 +243,10 @@ impl DatabaseService for RusqliteService {
async fn last_insert_rowid(&self) -> DbResult<i64> {
let conn = Arc::clone(&self.conn);
tokio::task::spawn_blocking(move || {
let conn = conn
.lock()
.map_err(|e| format!("Failed to lock connection: {}", e))?;
// `lock_safe`, not `lock`: this is the busiest lock in the app and a
// panic under the guard would otherwise poison it, failing every
// later query with "poisoned lock" until the process restarts.
let conn = conn.lock_safe();
Ok(conn.last_insert_rowid())
})
.await
@@ -410,4 +418,48 @@ mod tests {
let count: i32 = service.query_one(query, |row| row.get(0)).await.unwrap();
assert_eq!(count, 2);
}
/// A panic while the connection guard is held must not brick every later
/// query.
///
/// This is the single busiest lock in the app — every async DB operation
/// goes through it. With a raw `.lock()`, one panic under the guard poisons
/// the mutex and every subsequent call returns "poisoned lock" until the
/// process restarts, which for a database-backed app means the whole UI
/// stops working. `utils::lock` exists precisely to stop that cascade, and
/// `storage::Database` already used it; this path did not.
///
/// TRACES: UR-002 | DR-012 | UT-014
#[tokio::test]
async fn a_poisoned_connection_still_serves_queries() {
let conn = Arc::new(Mutex::new(Connection::open_in_memory().unwrap()));
{
let c = conn.lock_safe();
c.execute_batch("CREATE TABLE test (id INTEGER PRIMARY KEY);")
.unwrap();
}
// Poison the mutex the way a panicking row mapper would.
let poisoner = Arc::clone(&conn);
let hook = std::panic::take_hook();
std::panic::set_hook(Box::new(|_| {}));
let _ = std::thread::spawn(move || {
let _guard = poisoner.lock().unwrap();
panic!("a row mapper blew up while holding the connection");
})
.join();
std::panic::set_hook(hook);
assert!(conn.lock().is_err(), "the mutex should now be poisoned");
// Every operation must still work.
let service = RusqliteService::new(Arc::clone(&conn));
service
.execute(Query::new("INSERT INTO test (id) VALUES (1)"))
.await
.expect("execute must survive a poisoned connection");
let count: i32 = service
.query_one(Query::new("SELECT COUNT(*) FROM test"), |row| row.get(0))
.await
.expect("query_one must survive a poisoned connection");
assert_eq!(count, 1);
}
}
+139 -21
View File
@@ -75,8 +75,31 @@ impl Database {
Arc::clone(&self.conn)
}
/// Run all pending migrations
/// Run all pending migrations.
pub fn migrate(&self) -> SqliteResult<()> {
self.migrate_with(MIGRATIONS)
}
/// Apply `migrations` in order, skipping ones `_migrations` already records.
///
/// **Each migration is one transaction, and the `_migrations` row is written
/// inside it.** SQLite autocommits every statement otherwise, so a migration
/// that failed partway — low disk, an OOM kill, the process dying mid-boot —
/// used to leave its earlier statements applied while recording nothing.
/// `execute_batch` aborts on the first error, so the retry on the next launch
/// then failed at statement 1 ("duplicate column name") and kept failing
/// forever; `Database::open` turns that into a panic, so the app never
/// started again and the only fix was clearing app data. Committing the
/// schema change and the bookkeeping together makes a migration all-or-nothing
/// and a retry always safe.
///
/// Every migration is pure DDL/DML, which SQLite runs transactionally — a
/// `PRAGMA` or `VACUUM` added to one would not roll back and must not be.
///
/// Split out from [`Self::migrate`] so tests can inject a failing migration.
///
/// TRACES: UR-002 | DR-012 | UT-014
fn migrate_with(&self, migrations: &[(&str, &str)]) -> SqliteResult<()> {
info!("Starting database migrations...");
let conn = self.conn.lock_safe();
@@ -107,28 +130,30 @@ impl Database {
debug!("Found {} applied migrations", applied.len());
// Apply pending migrations
for (name, sql) in MIGRATIONS {
if !applied.contains(&name.to_string()) {
info!("Applying migration: {}", name);
match conn.execute_batch(sql) {
Ok(_) => {
info!("Successfully applied migration: {}", name);
match conn.execute("INSERT INTO _migrations (name) VALUES (?1)", [name]) {
Ok(_) => debug!("Recorded migration: {}", name),
Err(e) => {
error!("Failed to record migration {}: {}", name, e);
return Err(e);
}
}
}
Err(e) => {
error!("Failed to apply migration {}: {}", name, e);
return Err(e);
}
}
} else {
for (name, sql) in migrations {
if applied.contains(&name.to_string()) {
debug!("Skipping already applied migration: {}", name);
continue;
}
info!("Applying migration: {}", name);
// `unchecked_transaction` because the connection is reached through a
// shared guard rather than `&mut`. Dropping the transaction without
// committing rolls it back, which is exactly what the `?`s below do.
let tx = conn.unchecked_transaction()?;
if let Err(e) = tx.execute_batch(sql) {
error!("Failed to apply migration {} (rolled back): {}", name, e);
return Err(e);
}
if let Err(e) = tx.execute("INSERT INTO _migrations (name) VALUES (?1)", [name]) {
error!("Failed to record migration {} (rolled back): {}", name, e);
return Err(e);
}
tx.commit()?;
info!("Successfully applied migration: {}", name);
}
info!("All migrations completed successfully");
@@ -166,6 +191,99 @@ mod tests {
assert_eq!(db.path().to_str(), Some(":memory:"));
}
/// A migration that dies partway must leave *nothing* behind.
///
/// SQLite autocommits each statement, so before migrations were wrapped in a
/// transaction the first `ADD COLUMN` of a failing batch stuck while the
/// `_migrations` row was never written. `execute_batch` aborts on the first
/// error, so the retry on the next launch failed at statement 1 with
/// "duplicate column name" and kept failing forever — and `Database::open`
/// panics on that, so the app never started again.
///
/// TRACES: UR-002 | DR-012 | UT-014
#[test]
fn test_failed_migration_rolls_back_and_stays_retryable() {
let db = Database::open_in_memory().unwrap();
// Statement 1 succeeds, statement 2 fails, statement 3 never runs.
let poisoned = &[(
"900_partially_failing",
"ALTER TABLE downloads ADD COLUMN audit_a TEXT;
ALTER TABLE downloads ADD COLUMN audit_b TEXT FROM NOWHERE;
ALTER TABLE downloads ADD COLUMN audit_c TEXT;",
)][..];
let first = db.migrate_with(poisoned).unwrap_err();
// Nothing from the batch may survive, or the retry cannot re-run it.
assert!(
!has_column(&db, "downloads", "audit_a"),
"statement 1 of a failed migration was left applied: the batch did not roll back"
);
assert!(!has_column(&db, "downloads", "audit_c"));
// And it must not be recorded as applied.
let recorded: i64 = db
.connection()
.lock_safe()
.query_row(
"SELECT COUNT(*) FROM _migrations WHERE name = ?1",
["900_partially_failing"],
|r| r.get(0),
)
.unwrap();
assert_eq!(recorded, 0, "a failed migration must not be recorded");
// The retry must fail the same way it did the first time — reaching the
// real error — rather than tripping over its own leftovers.
let second = db.migrate_with(poisoned).unwrap_err();
assert!(
!second.to_string().contains("duplicate column"),
"the retry hit leftovers from the failed run instead of the real error: {second}"
);
assert_eq!(first.to_string(), second.to_string());
// A corrected migration under the same name then applies cleanly.
let fixed = &[(
"900_partially_failing",
"ALTER TABLE downloads ADD COLUMN audit_a TEXT;
ALTER TABLE downloads ADD COLUMN audit_c TEXT;",
)][..];
db.migrate_with(fixed).unwrap();
assert!(has_column(&db, "downloads", "audit_a"));
assert!(has_column(&db, "downloads", "audit_c"));
}
/// A committed migration is recorded, so it is never applied twice.
///
/// TRACES: UR-002 | DR-012 | UT-014
#[test]
fn test_successful_migration_is_recorded_in_the_same_transaction() {
let db = Database::open_in_memory().unwrap();
let m = &[(
"901_adds_a_column",
"ALTER TABLE downloads ADD COLUMN audit_d TEXT;",
)][..];
db.migrate_with(m).unwrap();
// Re-running must be a no-op, not a "duplicate column" failure.
db.migrate_with(m).unwrap();
assert!(has_column(&db, "downloads", "audit_d"));
}
fn has_column(db: &Database, table: &str, column: &str) -> bool {
let conn = db.connection();
let conn = conn.lock_safe();
let mut stmt = conn
.prepare(&format!("PRAGMA table_info({table})"))
.unwrap();
let mut names = stmt
.query_map([], |row| row.get::<_, String>(1))
.unwrap()
.filter_map(|r| r.ok());
names.any(|n| n == column)
}
#[test]
fn test_migrations_run() {
let db = Database::open_in_memory().unwrap();
+25
View File
@@ -29,6 +29,7 @@ pub const MIGRATIONS: &[(&str, &str)] = &[
("022_people_fts", MIGRATION_022),
("023_downloads_expiry", MIGRATION_023),
("024_multi_user_profiles", MIGRATION_024),
("025_backfill_item_library_id", MIGRATION_025),
];
/// Initial schema migration
@@ -896,6 +897,30 @@ INSERT OR IGNORE INTO download_grants (user_id, download_id)
SELECT d.user_id, d.id FROM downloads d;
"#;
/// Force cached items to be re-fetched so `library_id` is populated.
///
/// `save_to_cache` bound `library_id` NULL for every row it wrote, so nothing in
/// the cache knew which library it came from. The only available association was
/// the `collection_type` ↔ `item_type` taxonomy, which cannot tell two libraries
/// of the same type apart — a server with "TV" and "Shows" served both the same
/// contents — and says nothing at all about a library whose type it does not map
/// (Books, Photos, Collections, or a mixed library where Jellyfin sends no
/// collection type).
///
/// The write path now records the library. Existing rows cannot be repaired
/// locally — the association was never stored — so they are marked stale and
/// re-fetched on next browse, exactly as MIGRATION_018 did for `is_folder`.
///
/// Deliberately does not delete anything: downloads, favourites and playback
/// positions live in other tables and are untouched, and a cleared `synced_at`
/// only means "ask the server again", so an offline user keeps browsing what
/// they already had until the next successful fetch.
///
/// TRACES: UR-007 | DR-278
const MIGRATION_025: &str = r#"
UPDATE items SET synced_at = NULL;
"#;
#[cfg(test)]
mod migration_024_tests {
use super::*;
+1 -1
View File
@@ -1,7 +1,7 @@
{
"$schema": "https://schema.tauri.app/config/2",
"productName": "JellyTau",
"version": "0.11.5",
"version": "0.11.6",
"identifier": "com.dtourolle.jellytau",
"build": {
"beforeDevCommand": "bun run dev",