Compare commits
16
Commits
| Author | SHA1 | Date | |
|---|---|---|---|
|
|
4bce81a800 | ||
|
|
65d3d912f7 | ||
|
|
192a8b3c67 | ||
|
|
747ec0161c | ||
|
|
bc92eb4dea | ||
|
|
c72ca86865 | ||
|
|
f9e1a8e69a | ||
|
|
d4f80a4afa | ||
|
|
7b738002a0 | ||
|
|
c41b8ec896 | ||
|
|
dea78b89b9 | ||
|
|
368935e6f4 | ||
|
|
c44070e720 | ||
|
|
8ecf74a2af | ||
|
|
5fb9c1ff3b | ||
|
|
b5a3a3b427 |
@@ -1,27 +1,39 @@
|
||||
name: '📱 Test APK'
|
||||
|
||||
# An installable APK from any branch, on demand, without cutting a release.
|
||||
# Installable Android builds that are not releases.
|
||||
#
|
||||
# Why this exists separately from build-release.yml: that workflow is tag-driven,
|
||||
# builds Linux + Windows + Android and then *creates a release*, which is not
|
||||
# what you want from a feature branch. This builds one Android APK from whatever
|
||||
# ref you dispatch it on and hands it back as an artifact.
|
||||
# Two ways in:
|
||||
#
|
||||
# Deliberately `workflow_dispatch` only — no push trigger. The runner has a
|
||||
# single slot shared with two other projects, so a build on every feature-branch
|
||||
# commit would starve everything else. Dispatch it when you actually want to
|
||||
# install something.
|
||||
# push to master -> refreshes the rolling `latest` pre-release, so there is
|
||||
# always a current APK behind one stable URL that can be
|
||||
# handed to a tester once and never re-sent.
|
||||
# workflow_dispatch -> builds any branch on demand, optionally publishing it
|
||||
# as `test-<branch>`.
|
||||
#
|
||||
# Both variants install as com.dtourolle.jellytau.debug ("JellyTau Debug"),
|
||||
# side by side with a real install and with their own data directory. Neither
|
||||
# needs the release signing key.
|
||||
# Why this is separate from build-release.yml: that workflow is tag-driven,
|
||||
# builds Linux + Windows + Android and creates a real release. This produces one
|
||||
# APK and never touches the release channel.
|
||||
#
|
||||
# What comes out installs as com.dtourolle.jellytau.debug ("JellyTau Debug"),
|
||||
# side by side with a real install and with its own data directory. It is a
|
||||
# fully R8-minified release build -- minification is where Android builds have
|
||||
# actually broken here (R8 stripping JNI-loaded player and security classes),
|
||||
# and a plain debug build cannot catch that -- but it is signed with the debug
|
||||
# keystore rather than the store key. So a bad master commit can never replace
|
||||
# somebody's working install, and the production signing key stays in the
|
||||
# tag-driven workflow where it belongs.
|
||||
#
|
||||
# Getting the APK to somebody else: Gitea artifacts need an account with read
|
||||
# access to download, so `publish: true` also attaches the APK to a pre-release
|
||||
# whose assets are a plain public URL. That is the only way an outside tester
|
||||
# gets the file without being given an account.
|
||||
# access to download, so published builds are attached to a pre-release, whose
|
||||
# assets are a plain public URL. That is the only way an outside tester gets the
|
||||
# file without being given an account.
|
||||
|
||||
on:
|
||||
push:
|
||||
branches:
|
||||
- master
|
||||
paths-ignore:
|
||||
- '**/*.md'
|
||||
workflow_dispatch:
|
||||
inputs:
|
||||
variant:
|
||||
@@ -30,9 +42,7 @@ on:
|
||||
default: 'side-by-side-release'
|
||||
type: choice
|
||||
options:
|
||||
# R8-minified, exactly what ships, in the debug slot. Use this unless
|
||||
# you need stack traces: R8 stripping JNI-loaded classes has broken
|
||||
# release APKs here before, and a plain debug build cannot catch it.
|
||||
# R8-minified, exactly what ships, in the debug slot.
|
||||
- side-by-side-release
|
||||
# Unminified. Faster, readable stack traces, but does not exercise
|
||||
# minification at all.
|
||||
@@ -47,13 +57,15 @@ on:
|
||||
- armv7
|
||||
- x86_64
|
||||
publish:
|
||||
description: 'Also publish as a pre-release, for testers with no Gitea account'
|
||||
description: 'Also publish as a pre-release (automatic on master)'
|
||||
required: false
|
||||
default: false
|
||||
type: boolean
|
||||
|
||||
concurrency:
|
||||
# One test build at a time; a newer dispatch supersedes an in-flight one.
|
||||
# One APK build at a time, and a newer push supersedes an in-flight one — so a
|
||||
# burst of commits to master costs one build, not one per commit. This matters:
|
||||
# the runner has a single slot shared with two other projects.
|
||||
group: build-test-apk
|
||||
cancel-in-progress: true
|
||||
|
||||
@@ -63,8 +75,15 @@ env:
|
||||
|
||||
jobs:
|
||||
build:
|
||||
name: Build test APK (${{ inputs.variant }}, ${{ inputs.abi }})
|
||||
name: Build test APK
|
||||
runs-on: linux/amd64
|
||||
defaults:
|
||||
run:
|
||||
# This runner executes `run:` blocks with `sh` (dash) unless told
|
||||
# otherwise, so bash-only syntax fails with a bare "Bad substitution"
|
||||
# naming a temp file and no line of your workflow. Say bash explicitly.
|
||||
# The short-SHA output below avoids depending on it regardless.
|
||||
shell: bash
|
||||
container:
|
||||
image: gitea.tourolle.paris/dtourolle/jellytau-builder:2026.08.1
|
||||
env:
|
||||
@@ -79,6 +98,69 @@ jobs:
|
||||
# the tags have to be here. A shallow checkout yields 0.0.0.
|
||||
fetch-depth: 0
|
||||
|
||||
# One place decides what this run is, so the build, the collect step and
|
||||
# the publish step cannot disagree about it. A push carries no dispatch
|
||||
# inputs at all -- every `github.event.inputs.*` is empty on that event --
|
||||
# so each value needs an explicit default rather than being read raw.
|
||||
- name: Resolve build parameters
|
||||
id: cfg
|
||||
run: |
|
||||
set -e
|
||||
VARIANT="${{ github.event.inputs.variant }}"
|
||||
ABI="${{ github.event.inputs.abi }}"
|
||||
PUBLISH="${{ github.event.inputs.publish }}"
|
||||
BRANCH="${GITHUB_REF#refs/heads/}"
|
||||
|
||||
VARIANT="${VARIANT:-side-by-side-release}"
|
||||
ABI="${ABI:-aarch64}"
|
||||
|
||||
# A push to master always publishes -- that is the whole point of a
|
||||
# rolling `latest`. A dispatch publishes only if asked. Compared
|
||||
# against the string 'true' rather than used as a bare truthiness
|
||||
# test: dispatch inputs arrive as strings, and every non-empty string
|
||||
# is truthy, so `if: inputs.publish` would publish even when the box
|
||||
# was deliberately left unticked.
|
||||
if [ "$GITHUB_EVENT_NAME" = "push" ]; then
|
||||
PUBLISH=true
|
||||
elif [ "$PUBLISH" = "true" ]; then
|
||||
PUBLISH=true
|
||||
else
|
||||
PUBLISH=false
|
||||
fi
|
||||
|
||||
# Master is the rolling channel and keeps one stable tag, so the
|
||||
# download URL a tester was given keeps working. Anything else gets
|
||||
# its own branch-scoped tag.
|
||||
if [ "$BRANCH" = "master" ]; then
|
||||
TAG="latest"
|
||||
RELEASE_NAME="Latest build (master)"
|
||||
else
|
||||
TAG="test-$(echo "$BRANCH" | tr '/' '-')"
|
||||
RELEASE_NAME="Test build: $BRANCH"
|
||||
fi
|
||||
|
||||
# Stable asset name for the same reason the tag is stable.
|
||||
ASSET="jellytau-${TAG}.apk"
|
||||
|
||||
# Computed once, with `cut` rather than `${GITHUB_SHA::8}`. The
|
||||
# substring form is bash-only and this runner may hand a step to
|
||||
# `sh`; that cost a 51-minute build which produced a perfectly good
|
||||
# APK and then died formatting the summary table.
|
||||
SHORT_SHA=$(printf '%s' "$GITHUB_SHA" | cut -c1-8)
|
||||
|
||||
{
|
||||
echo "variant=$VARIANT"
|
||||
echo "abi=$ABI"
|
||||
echo "publish=$PUBLISH"
|
||||
echo "tag=$TAG"
|
||||
echo "release_name=$RELEASE_NAME"
|
||||
echo "asset=$ASSET"
|
||||
echo "branch=$BRANCH"
|
||||
echo "short_sha=$SHORT_SHA"
|
||||
} >> "$GITHUB_OUTPUT"
|
||||
|
||||
echo "variant=$VARIANT abi=$ABI publish=$PUBLISH tag=$TAG asset=$ASSET"
|
||||
|
||||
- name: Cache Rust dependencies
|
||||
uses: actions/cache@v3
|
||||
with:
|
||||
@@ -129,30 +211,29 @@ jobs:
|
||||
# APK actually carries, which has silently regressed before.
|
||||
- name: Build APK
|
||||
run: |
|
||||
if [ "${{ inputs.variant }}" = "side-by-side-release" ]; then
|
||||
./scripts/build-android.sh release --debug --abi "${{ inputs.abi }}"
|
||||
if [ "${{ steps.cfg.outputs.variant }}" = "side-by-side-release" ]; then
|
||||
./scripts/build-android.sh release --debug --abi "${{ steps.cfg.outputs.abi }}"
|
||||
else
|
||||
./scripts/build-android.sh debug --abi "${{ inputs.abi }}"
|
||||
./scripts/build-android.sh debug --abi "${{ steps.cfg.outputs.abi }}"
|
||||
fi
|
||||
|
||||
- name: Collect APK
|
||||
id: collect
|
||||
run: |
|
||||
set -e
|
||||
mkdir -p dist/test-apk
|
||||
if [ "${{ inputs.variant }}" = "side-by-side-release" ]; then
|
||||
if [ "${{ steps.cfg.outputs.variant }}" = "side-by-side-release" ]; then
|
||||
PATTERN='*-release.apk'
|
||||
else
|
||||
PATTERN='*-debug.apk'
|
||||
fi
|
||||
APK=$(find src-tauri/gen/android/app/build/outputs/apk -name "$PATTERN" | head -1)
|
||||
if [ -z "$APK" ]; then
|
||||
echo "❌ No APK produced for variant ${{ inputs.variant }}"
|
||||
echo "❌ No APK produced for variant ${{ steps.cfg.outputs.variant }}"
|
||||
find src-tauri/gen/android/app/build/outputs/apk -name '*.apk' || true
|
||||
exit 1
|
||||
fi
|
||||
|
||||
REF_NAME=$(echo "${GITHUB_REF#refs/heads/}" | tr '/' '-')
|
||||
OUT="dist/test-apk/jellytau-${REF_NAME}-${GITHUB_SHA::8}-${{ inputs.variant }}.apk"
|
||||
OUT="dist/test-apk/${{ steps.cfg.outputs.asset }}"
|
||||
cp "$APK" "$OUT"
|
||||
|
||||
# Report what the thing actually is, not what it was meant to be.
|
||||
@@ -160,37 +241,32 @@ jobs:
|
||||
"$APKSIGNER" verify --print-certs "$OUT" || echo "⚠️ Could not verify signature"
|
||||
|
||||
{
|
||||
echo "### 📱 Test APK"
|
||||
echo "### 📱 ${{ steps.cfg.outputs.release_name }}"
|
||||
echo ""
|
||||
echo "| | |"
|
||||
echo "|---|---|"
|
||||
echo "| Branch | \`${GITHUB_REF#refs/heads/}\` |"
|
||||
echo "| Commit | \`${GITHUB_SHA::8}\` |"
|
||||
echo "| Variant | \`${{ inputs.variant }}\` |"
|
||||
echo "| ABI | \`${{ inputs.abi }}\` |"
|
||||
echo "| Branch | \`${{ steps.cfg.outputs.branch }}\` |"
|
||||
echo "| Commit | \`${{ steps.cfg.outputs.short_sha }}\` |"
|
||||
echo "| Variant | \`${{ steps.cfg.outputs.variant }}\` |"
|
||||
echo "| ABI | \`${{ steps.cfg.outputs.abi }}\` |"
|
||||
echo "| Size | $(du -h "$OUT" | cut -f1) |"
|
||||
echo "| SHA256 | \`$(sha256sum "$OUT" | cut -d' ' -f1)\` |"
|
||||
echo ""
|
||||
echo "Installs as \`com.dtourolle.jellytau.debug\` — side by side with a real"
|
||||
echo "install, with its own data directory. Download the artifact, then:"
|
||||
echo ""
|
||||
echo '```'
|
||||
echo "adb install -r $(basename "$OUT")"
|
||||
echo '```'
|
||||
} >> "$GITHUB_STEP_SUMMARY"
|
||||
|
||||
ls -lah dist/test-apk/
|
||||
|
||||
# Deliberately NOT tagged `v*`: that pattern triggers build-release.yml,
|
||||
# which would run the whole three-platform release matrix and publish a
|
||||
# real release off a feature branch. The tag here is derived from the
|
||||
# branch name and carries no version, so nothing else reacts to it.
|
||||
# real release. `latest` and `test-*` carry no version, so nothing else
|
||||
# reacts to them.
|
||||
#
|
||||
# This also cannot reach existing users. The desktop updater reads a
|
||||
# static latest.json from the `updater` branch, not the release list, so a
|
||||
# pre-release published here is invisible to anyone without the link.
|
||||
- name: Publish as a pre-release
|
||||
if: ${{ inputs.publish }}
|
||||
# This also cannot reach existing users by itself. The desktop updater
|
||||
# reads a static latest.json from the `updater` branch, not the release
|
||||
# list, so a pre-release published here is invisible to anyone who does
|
||||
# not have the link -- and the APK installs under a different
|
||||
# applicationId anyway.
|
||||
- name: Publish pre-release
|
||||
if: ${{ steps.cfg.outputs.publish == 'true' }}
|
||||
env:
|
||||
GITEA_TOKEN: ${{ secrets.GITEA_TOKEN }}
|
||||
AUTO_TOKEN: ${{ secrets.GITHUB_TOKEN }}
|
||||
@@ -200,26 +276,26 @@ jobs:
|
||||
API="${GITHUB_SERVER_URL}/api/v1"
|
||||
REPO="${GITHUB_REPOSITORY}"
|
||||
TOKEN="${GITEA_TOKEN:-$AUTO_TOKEN}"
|
||||
BRANCH="${GITHUB_REF#refs/heads/}"
|
||||
TAG="test-$(echo "$BRANCH" | tr '/' '-')"
|
||||
TAG="${{ steps.cfg.outputs.tag }}"
|
||||
ASSET="${{ steps.cfg.outputs.asset }}"
|
||||
|
||||
# printf, not a heredoc: inside a YAML block scalar every line is
|
||||
# indented, and a heredoc terminator has to sit at column 0.
|
||||
BODY=$(printf '%s\n' \
|
||||
"Test build of \`$BRANCH\` at \`${GITHUB_SHA::8}\` — **not a release**." \
|
||||
"Automatic build of \`${{ steps.cfg.outputs.branch }}\` at \`${{ steps.cfg.outputs.short_sha }}\` — **not a release**." \
|
||||
"" \
|
||||
"Installs as **JellyTau Debug** (\`com.dtourolle.jellytau.debug\`), alongside a" \
|
||||
"normal install and with its own separate data. Uninstalling it does not touch" \
|
||||
"the real app." \
|
||||
"normal install and with its own separate data. It cannot replace or upgrade a" \
|
||||
"real install, and uninstalling it does not touch one." \
|
||||
"" \
|
||||
"Variant: \`${{ inputs.variant }}\` · ABI: \`${{ inputs.abi }}\`" \
|
||||
"R8-minified like a real release, but signed with a debug key — so Android will" \
|
||||
"warn about an unknown source. That is expected." \
|
||||
"" \
|
||||
"Android will warn about installing from an unknown source; that is expected" \
|
||||
"for a build signed with a debug key rather than the store key.")
|
||||
"Variant: \`${{ steps.cfg.outputs.variant }}\` · ABI: \`${{ steps.cfg.outputs.abi }}\`" \
|
||||
"" \
|
||||
"This release is refreshed on every push; the download link stays the same.")
|
||||
|
||||
PAYLOAD=$(jq -n \
|
||||
--arg tag "$TAG" \
|
||||
--arg name "Test build: $BRANCH" \
|
||||
--arg name "${{ steps.cfg.outputs.release_name }}" \
|
||||
--arg body "$BODY" \
|
||||
--arg target "$GITHUB_SHA" \
|
||||
'{tag_name:$tag, target_commitish:$target, name:$name, body:$body, draft:false, prerelease:true}')
|
||||
@@ -230,11 +306,14 @@ jobs:
|
||||
if [ "$HTTP" = "201" ]; then
|
||||
RELEASE_ID=$(jq -r '.id' resp.json)
|
||||
elif [ "$HTTP" = "409" ]; then
|
||||
# Re-dispatching for the same branch replaces the previous APK rather
|
||||
# than accumulating one release per attempt.
|
||||
echo "ℹ️ Pre-release $TAG exists; reusing it"
|
||||
# The rolling case: reuse the release, refresh its body to name the
|
||||
# new commit, and clear the old asset so `latest` means latest.
|
||||
RELEASE_ID=$(curl -fsS "$API/repos/$REPO/releases/tags/$TAG" \
|
||||
-H "Authorization: token $TOKEN" | jq -r '.id')
|
||||
echo "ℹ️ Refreshing existing pre-release $TAG (id=$RELEASE_ID)"
|
||||
curl -fsS -X PATCH "$API/repos/$REPO/releases/$RELEASE_ID" \
|
||||
-H "Authorization: token $TOKEN" -H "Content-Type: application/json" \
|
||||
-d "$PAYLOAD" >/dev/null
|
||||
for id in $(curl -fsS "$API/repos/$REPO/releases/$RELEASE_ID/assets" \
|
||||
-H "Authorization: token $TOKEN" | jq -r '.[].id'); do
|
||||
curl -fsS -X DELETE "$API/repos/$REPO/releases/$RELEASE_ID/assets/$id" \
|
||||
@@ -244,21 +323,24 @@ jobs:
|
||||
echo "❌ Failed to create pre-release (HTTP $HTTP):"; cat resp.json; exit 1
|
||||
fi
|
||||
|
||||
for f in dist/test-apk/*.apk; do
|
||||
echo "⬆️ $(basename "$f")"
|
||||
curl -fsS -X POST \
|
||||
"$API/repos/$REPO/releases/$RELEASE_ID/assets?name=$(basename "$f")" \
|
||||
-H "Authorization: token $TOKEN" -F "attachment=@$f" >/dev/null
|
||||
done
|
||||
# The tag moves with the branch, so an old tag object would otherwise
|
||||
# keep `latest` pointing at a stale commit.
|
||||
curl -fsS -X POST \
|
||||
"$API/repos/$REPO/releases/$RELEASE_ID/assets?name=$ASSET" \
|
||||
-H "Authorization: token $TOKEN" -F "attachment=@dist/test-apk/$ASSET" >/dev/null
|
||||
|
||||
URL="${GITHUB_SERVER_URL}/${REPO}/releases/download/${TAG}/${ASSET}"
|
||||
{
|
||||
echo ""
|
||||
echo "**Published:** ${GITHUB_SERVER_URL}/${REPO}/releases/tag/${TAG}"
|
||||
echo ""
|
||||
echo "Public link — no Gitea account needed. Delete the release when testing is done."
|
||||
echo "Direct download (stable link, no account needed):"
|
||||
echo ""
|
||||
echo " $URL"
|
||||
} >> "$GITHUB_STEP_SUMMARY"
|
||||
echo "✅ Published $TAG -> $URL"
|
||||
|
||||
- name: Upload APK
|
||||
- name: Upload APK artifact
|
||||
uses: actions/upload-artifact@v3
|
||||
with:
|
||||
name: jellytau-test-apk
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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
@@ -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",
|
||||
|
||||
@@ -7,7 +7,7 @@ echo "======================================"
|
||||
# Setup environment
|
||||
echo "Setting up environment..."
|
||||
source "$HOME/.cargo/env.fish" 2>/dev/null || source "$HOME/.cargo/env" || true
|
||||
export ANDROID_HOME="$HOME/Android/Sdk"
|
||||
export ANDROID_HOME="${ANDROID_HOME:-$HOME/Android/Sdk}"
|
||||
export NDK_HOME="$ANDROID_HOME/ndk/$(ls $ANDROID_HOME/ndk 2>/dev/null | head -1)"
|
||||
|
||||
# Check prerequisites
|
||||
|
||||
@@ -6,9 +6,26 @@ set -e
|
||||
# Source Rust environment
|
||||
source "$HOME/.cargo/env.fish" 2>/dev/null || source "$HOME/.cargo/env" 2>/dev/null || true
|
||||
|
||||
# Set Android environment variables
|
||||
export ANDROID_HOME="$HOME/Android/Sdk"
|
||||
export NDK_HOME="$ANDROID_HOME/ndk/$(ls "$ANDROID_HOME/ndk" | head -1)"
|
||||
# Set Android environment variables.
|
||||
#
|
||||
# Defaults, not overrides. A developer's SDK is at ~/Android/Sdk, but CI runs in
|
||||
# the builder image where it lives at /opt/android-sdk and the job sets
|
||||
# ANDROID_HOME accordingly — hardcoding the home-directory path here silently
|
||||
# discarded that and the build died with "Android SDK not found" a minute in.
|
||||
# `test-player-conformance.sh` already had this right; this script did not.
|
||||
export ANDROID_HOME="${ANDROID_HOME:-$HOME/Android/Sdk}"
|
||||
export ANDROID_SDK_ROOT="${ANDROID_SDK_ROOT:-$ANDROID_HOME}"
|
||||
|
||||
if [ ! -d "$ANDROID_HOME/ndk" ]; then
|
||||
echo "❌ No NDK directory at $ANDROID_HOME/ndk" >&2
|
||||
echo " Set ANDROID_HOME to your SDK location, or install the NDK." >&2
|
||||
exit 1
|
||||
fi
|
||||
|
||||
# Respect an NDK the caller has already picked (CI pins an exact revision via
|
||||
# ANDROID_NDK_HOME); otherwise take whatever is installed.
|
||||
export NDK_HOME="${NDK_HOME:-${ANDROID_NDK_HOME:-$ANDROID_HOME/ndk/$(ls "$ANDROID_HOME/ndk" | head -1)}}"
|
||||
export ANDROID_NDK_HOME="$NDK_HOME"
|
||||
|
||||
echo "🤖 Building Android APK..."
|
||||
echo "Android SDK: $ANDROID_HOME"
|
||||
|
||||
Generated
+1
-1
@@ -2209,7 +2209,7 @@ dependencies = [
|
||||
|
||||
[[package]]
|
||||
name = "jellytau"
|
||||
version = "0.11.5"
|
||||
version = "0.11.6"
|
||||
dependencies = [
|
||||
"aes-gcm",
|
||||
"argon2",
|
||||
|
||||
@@ -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"
|
||||
|
||||
@@ -94,40 +94,56 @@ Follow the right log stream with `./scripts/logcat.sh [debug|release]`
|
||||
|
||||
### Getting a test APK out of CI
|
||||
|
||||
`.gitea/workflows/build-test-apk.yml` builds one from **any branch, on demand**
|
||||
— run it from Gitea's Actions tab (`workflow_dispatch`) against the ref you want.
|
||||
It is not a release: nothing is tagged, published, or signed with the real key.
|
||||
`.gitea/workflows/build-test-apk.yml` produces installable APKs that are **not
|
||||
releases**. Two ways in:
|
||||
|
||||
Two variants, both installing into the `com.dtourolle.jellytau.debug` slot:
|
||||
| Trigger | Result |
|
||||
|---------|--------|
|
||||
| **push to `master`** | Refreshes the rolling **`latest`** pre-release automatically |
|
||||
| **`workflow_dispatch`** | Builds any branch on demand; optionally publishes it as `test-<branch>` |
|
||||
|
||||
#### The rolling `latest` build
|
||||
|
||||
Every push to `master` (bar doc-only ones) rebuilds and replaces the APK on the
|
||||
`latest` pre-release. Both the tag and the asset name are stable, so the
|
||||
download URL never changes:
|
||||
|
||||
```
|
||||
https://gitea.tourolle.paris/dtourolle/jellytau/releases/download/latest/jellytau-latest.apk
|
||||
```
|
||||
|
||||
Send that link to a tester once and it keeps serving the current build. No
|
||||
account needed — release assets are public, unlike Actions artifacts.
|
||||
|
||||
#### What you get, and why it is safe
|
||||
|
||||
Both variants install into the `com.dtourolle.jellytau.debug` slot:
|
||||
|
||||
| Variant | What it is | When |
|
||||
|---------|-----------|------|
|
||||
| `side-by-side-release` (default) | R8-minified, exactly what ships, signed with the debug keystore | Almost always — a plain debug build cannot catch R8 stripping JNI-loaded classes, which has broken release APKs here before |
|
||||
| `side-by-side-release` (default, and what `latest` always is) | R8-minified, exactly what ships, signed with the **debug** keystore | Almost always — a plain debug build cannot catch R8 stripping JNI-loaded classes, which has broken release APKs here before |
|
||||
| `debug` | Unminified | When you need readable stack traces |
|
||||
|
||||
There is deliberately **no push trigger**: the runner has one slot shared with
|
||||
two other projects, so building on every feature-branch commit would starve
|
||||
them. The APK lands as the `jellytau-test-apk` artifact (7-day retention), named
|
||||
for the branch and short SHA, with its size and SHA256 in the run summary.
|
||||
Three properties make an automatic build on every master push safe:
|
||||
|
||||
#### Sending a build to an outside tester
|
||||
- **It cannot replace a real install.** The applicationId is suffixed `.debug`,
|
||||
so it sits beside the store build with its own data. A broken master commit
|
||||
can never take out somebody's working app.
|
||||
- **The production signing key is not involved.** That stays in the tag-driven
|
||||
`build-release.yml`. This workflow needs no secrets beyond the API token.
|
||||
- **The tag is `latest`/`test-*`, never `v*`.** Only `v*` triggers
|
||||
`build-release.yml`. And the desktop updater reads a static `latest.json` from
|
||||
the `updater` branch rather than the release list, so nothing here is offered
|
||||
to existing users.
|
||||
|
||||
Gitea **artifacts require an account** with read access to download, so an
|
||||
artifact is no use to someone outside the project. Tick **`publish`** on the
|
||||
dispatch and the APK is also attached to a **pre-release**, whose assets are a
|
||||
plain public URL on a public repo — no account, no MR, no merge to `master`.
|
||||
**Known gap:** the APK builds in parallel with `build-and-test.yml`, not after
|
||||
it, so `latest` can carry a commit whose tests later fail. Cross-workflow
|
||||
dependencies are not reliably available here, and duplicating the test job would
|
||||
double an already hour-long queue on a single-slot runner. Check the commit's
|
||||
CI status before handing the link to somebody.
|
||||
|
||||
Two things make that safe to do from a feature branch:
|
||||
|
||||
- The tag is `test-<branch>`, **not** `v*`. Only `v*` triggers
|
||||
`build-release.yml`, so nothing else reacts to it.
|
||||
- It cannot reach existing users. The desktop updater reads a static
|
||||
`latest.json` from the `updater` branch, not the release list, so a
|
||||
pre-release published this way is invisible to anyone without the link.
|
||||
|
||||
Re-dispatching for the same branch replaces the APK on the existing
|
||||
pre-release rather than piling up one release per attempt. Delete the release
|
||||
when testing is over.
|
||||
Runs are serialised and `cancel-in-progress` is on, so a burst of pushes to
|
||||
master collapses into one build rather than one per commit.
|
||||
|
||||
### Key Files
|
||||
|
||||
|
||||
+296
-50
@@ -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);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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.
|
||||
|
||||
+379
-313
@@ -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,42 +678,47 @@ pub extern "system" fn Java_com_dtourolle_jellytau_player_JellyTauPlayer_nativeO
|
||||
position: jdouble,
|
||||
duration: jdouble,
|
||||
) {
|
||||
// Debug: Log every 10th update to avoid spam
|
||||
static mut UPDATE_COUNTER: u32 = 0;
|
||||
unsafe {
|
||||
UPDATE_COUNTER += 1;
|
||||
if UPDATE_COUNTER % 10 == 0 {
|
||||
log::debug!("[Android] Position update {} / {}", position, duration);
|
||||
}
|
||||
}
|
||||
jni_guard(
|
||||
"Java_com_dtourolle_jellytau_player_JellyTauPlayer_nativeOnPositionUpdate",
|
||||
|| {
|
||||
// Debug: Log every 10th update to avoid spam
|
||||
static mut UPDATE_COUNTER: u32 = 0;
|
||||
unsafe {
|
||||
UPDATE_COUNTER += 1;
|
||||
if UPDATE_COUNTER % 10 == 0 {
|
||||
log::debug!("[Android] Position update {} / {}", position, duration);
|
||||
}
|
||||
}
|
||||
|
||||
// Update state and get the preserved duration to emit
|
||||
let duration_to_emit = if let Some(state) = SHARED_STATE.get() {
|
||||
let mut state = state.lock_safe();
|
||||
state.position = position;
|
||||
if duration > 0.0 {
|
||||
state.duration = Some(duration);
|
||||
}
|
||||
// Use preserved duration from state, or fall back to the received value
|
||||
state.duration.unwrap_or(duration)
|
||||
} else {
|
||||
duration
|
||||
};
|
||||
// Update state and get the preserved duration to emit
|
||||
let duration_to_emit = if let Some(state) = SHARED_STATE.get() {
|
||||
let mut state = state.lock_safe();
|
||||
state.position = position;
|
||||
if duration > 0.0 {
|
||||
state.duration = Some(duration);
|
||||
}
|
||||
// Use preserved duration from state, or fall back to the received value
|
||||
state.duration.unwrap_or(duration)
|
||||
} else {
|
||||
duration
|
||||
};
|
||||
|
||||
if let Some(emitter) = EVENT_EMITTER.get() {
|
||||
emitter.emit(PlayerStatusEvent::PositionUpdate {
|
||||
position,
|
||||
duration: duration_to_emit,
|
||||
});
|
||||
} else {
|
||||
log::error!("[Android] WARNING: No event emitter for position update!");
|
||||
}
|
||||
if let Some(emitter) = EVENT_EMITTER.get() {
|
||||
emitter.emit(PlayerStatusEvent::PositionUpdate {
|
||||
position,
|
||||
duration: duration_to_emit,
|
||||
});
|
||||
} else {
|
||||
log::error!("[Android] WARNING: No event emitter for position update!");
|
||||
}
|
||||
|
||||
// Throttled progress reporting to Jellyfin so playback position syncs and can
|
||||
// be resumed on another device. ExoPlayer only fires position updates while
|
||||
// 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);
|
||||
// Throttled progress reporting to Jellyfin so playback position syncs and can
|
||||
// be resumed on another device. ExoPlayer only fires position updates while
|
||||
// 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,51 +805,56 @@ pub extern "system" fn Java_com_dtourolle_jellytau_player_JellyTauPlayer_nativeO
|
||||
state: JString,
|
||||
media_id: JString,
|
||||
) {
|
||||
let state_str: String = env.get_string(&state).map(|s| s.into()).unwrap_or_default();
|
||||
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() {
|
||||
None
|
||||
} else {
|
||||
env.get_string(&media_id).map(|s| s.into()).ok()
|
||||
};
|
||||
let media_id_opt: Option<String> = if media_id.is_null() {
|
||||
None
|
||||
} else {
|
||||
env.get_string(&media_id).map(|s| s.into()).ok()
|
||||
};
|
||||
|
||||
// Update shared state
|
||||
if let Some(shared) = SHARED_STATE.get() {
|
||||
let mut shared = shared.lock_safe();
|
||||
if let Some(media) = shared.current_media.clone() {
|
||||
let duration = shared.duration.unwrap_or(0.0);
|
||||
let position = shared.position;
|
||||
match state_str.as_str() {
|
||||
"playing" => {
|
||||
shared.state = PlayerState::Playing {
|
||||
media,
|
||||
position,
|
||||
duration,
|
||||
};
|
||||
shared.is_loaded = true;
|
||||
// Update shared state
|
||||
if let Some(shared) = SHARED_STATE.get() {
|
||||
let mut shared = shared.lock_safe();
|
||||
if let Some(media) = shared.current_media.clone() {
|
||||
let duration = shared.duration.unwrap_or(0.0);
|
||||
let position = shared.position;
|
||||
match state_str.as_str() {
|
||||
"playing" => {
|
||||
shared.state = PlayerState::Playing {
|
||||
media,
|
||||
position,
|
||||
duration,
|
||||
};
|
||||
shared.is_loaded = true;
|
||||
}
|
||||
"paused" => {
|
||||
shared.state = PlayerState::Paused {
|
||||
media,
|
||||
position,
|
||||
duration,
|
||||
};
|
||||
}
|
||||
"idle" => {
|
||||
shared.state = PlayerState::Idle;
|
||||
shared.is_loaded = false;
|
||||
}
|
||||
_ => {}
|
||||
}
|
||||
}
|
||||
"paused" => {
|
||||
shared.state = PlayerState::Paused {
|
||||
media,
|
||||
position,
|
||||
duration,
|
||||
};
|
||||
}
|
||||
"idle" => {
|
||||
shared.state = PlayerState::Idle;
|
||||
shared.is_loaded = false;
|
||||
}
|
||||
_ => {}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
if let Some(emitter) = EVENT_EMITTER.get() {
|
||||
emitter.emit(PlayerStatusEvent::StateChanged {
|
||||
state: state_str,
|
||||
media_id: media_id_opt,
|
||||
});
|
||||
}
|
||||
if let Some(emitter) = EVENT_EMITTER.get() {
|
||||
emitter.emit(PlayerStatusEvent::StateChanged {
|
||||
state: state_str,
|
||||
media_id: media_id_opt,
|
||||
});
|
||||
}
|
||||
},
|
||||
);
|
||||
}
|
||||
|
||||
/// Called when media has finished loading.
|
||||
@@ -844,15 +864,20 @@ pub extern "system" fn Java_com_dtourolle_jellytau_player_JellyTauPlayer_nativeO
|
||||
_class: JClass,
|
||||
duration: jdouble,
|
||||
) {
|
||||
if let Some(state) = SHARED_STATE.get() {
|
||||
let mut state = state.lock_safe();
|
||||
state.duration = Some(duration);
|
||||
state.is_loaded = true;
|
||||
}
|
||||
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);
|
||||
state.is_loaded = true;
|
||||
}
|
||||
|
||||
if let Some(emitter) = EVENT_EMITTER.get() {
|
||||
emitter.emit(PlayerStatusEvent::MediaLoaded { duration });
|
||||
}
|
||||
if let Some(emitter) = EVENT_EMITTER.get() {
|
||||
emitter.emit(PlayerStatusEvent::MediaLoaded { duration });
|
||||
}
|
||||
},
|
||||
);
|
||||
}
|
||||
|
||||
/// Called when playback reaches the end.
|
||||
@@ -861,49 +886,38 @@ pub extern "system" fn Java_com_dtourolle_jellytau_player_JellyTauPlayer_nativeO
|
||||
_env: JNIEnv,
|
||||
_class: JClass,
|
||||
) {
|
||||
log::info!("[ExoPlayer] Playback ended - processing autoplay decision");
|
||||
jni_guard(
|
||||
"Java_com_dtourolle_jellytau_player_JellyTauPlayer_nativeOnPlaybackEnded",
|
||||
|| {
|
||||
log::info!("[ExoPlayer] Playback ended - processing autoplay decision");
|
||||
|
||||
// Get player controller and handle autoplay decision
|
||||
if let Some(controller) = PLAYER_CONTROLLER.get() {
|
||||
let controller = controller.clone();
|
||||
// Get player controller and handle autoplay decision
|
||||
if let Some(controller) = PLAYER_CONTROLLER.get() {
|
||||
let controller = controller.clone();
|
||||
|
||||
// Spawn async task to handle autoplay decision
|
||||
// Use tauri::async_runtime::spawn instead of tokio::spawn
|
||||
// JNI callbacks happen on arbitrary threads without a Tokio runtime
|
||||
tauri::async_runtime::spawn(async move {
|
||||
// Compute the autoplay decision and release the lock before matching.
|
||||
// Holding the guard across the match would deadlock the AdvanceToNext
|
||||
// arm, which re-locks the controller to call next() — leaving playback
|
||||
// stopped (paused at position 0) instead of advancing.
|
||||
let decision = controller.lock().await.on_playback_ended().await;
|
||||
match decision {
|
||||
Ok(AutoplayDecision::Stop) => {
|
||||
log::debug!("[Autoplay] Decision: Stop playback");
|
||||
// Emit PlaybackEnded event to frontend
|
||||
if let Some(emitter) = EVENT_EMITTER.get() {
|
||||
emitter.emit(PlayerStatusEvent::PlaybackEnded);
|
||||
}
|
||||
}
|
||||
Ok(AutoplayDecision::AdvanceToNext) => {
|
||||
log::debug!("[Autoplay] Decision: Advance to next track");
|
||||
// Advance to next track in queue
|
||||
let ctrl = controller.lock().await;
|
||||
// Spawn async task to handle autoplay decision
|
||||
// Use tauri::async_runtime::spawn instead of tokio::spawn
|
||||
// JNI callbacks happen on arbitrary threads without a Tokio runtime
|
||||
tauri::async_runtime::spawn(async move {
|
||||
// Compute the autoplay decision and release the lock before matching.
|
||||
// Holding the guard across the match would deadlock the AdvanceToNext
|
||||
// arm, which re-locks the controller to call next() — leaving playback
|
||||
// stopped (paused at position 0) instead of advancing.
|
||||
let decision = controller.lock().await.on_playback_ended().await;
|
||||
match decision {
|
||||
Ok(AutoplayDecision::Stop) => {
|
||||
log::debug!("[Autoplay] Decision: Stop playback");
|
||||
// Emit PlaybackEnded event to frontend
|
||||
if let Some(emitter) = EVENT_EMITTER.get() {
|
||||
emitter.emit(PlayerStatusEvent::PlaybackEnded);
|
||||
}
|
||||
}
|
||||
Ok(AutoplayDecision::AdvanceToNext) => {
|
||||
log::debug!("[Autoplay] Decision: Advance to next track");
|
||||
// Advance to next track in queue
|
||||
let ctrl = controller.lock().await;
|
||||
|
||||
// Log queue state before advancing
|
||||
let queue_info = {
|
||||
let queue = ctrl.queue.lock_safe();
|
||||
format!(
|
||||
"current_index={:?}, len={}",
|
||||
queue.current_index(),
|
||||
queue.items().len()
|
||||
)
|
||||
};
|
||||
log::debug!("[Autoplay] Queue state before next(): {}", queue_info);
|
||||
|
||||
match ctrl.next() {
|
||||
Ok(_) => {
|
||||
log::info!("[Autoplay] Successfully advanced to next track");
|
||||
// Log queue state after advancing
|
||||
// Log queue state before advancing
|
||||
let queue_info = {
|
||||
let queue = ctrl.queue.lock_safe();
|
||||
format!(
|
||||
@@ -912,93 +926,115 @@ 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 before next(): {}", queue_info);
|
||||
|
||||
// Emit queue changed event so frontend updates UI with new current track
|
||||
ctrl.emit_queue_changed();
|
||||
match ctrl.next() {
|
||||
Ok(_) => {
|
||||
log::info!("[Autoplay] Successfully advanced to next track");
|
||||
// Log queue state after advancing
|
||||
let queue_info = {
|
||||
let queue = ctrl.queue.lock_safe();
|
||||
format!(
|
||||
"current_index={:?}, len={}",
|
||||
queue.current_index(),
|
||||
queue.items().len()
|
||||
)
|
||||
};
|
||||
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
|
||||
);
|
||||
// Emit PlaybackEnded event on error
|
||||
if let Some(emitter) = EVENT_EMITTER.get() {
|
||||
emitter.emit(PlayerStatusEvent::PlaybackEnded);
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
Ok(AutoplayDecision::ShowNextEpisodePopup {
|
||||
current_episode,
|
||||
next_episode,
|
||||
countdown_seconds,
|
||||
auto_advance,
|
||||
}) => {
|
||||
log::info!(
|
||||
"[Autoplay] Decision: Show next episode popup (countdown: {}s, auto: {})",
|
||||
countdown_seconds,
|
||||
auto_advance
|
||||
);
|
||||
|
||||
// Emit popup event to frontend
|
||||
if let Some(emitter) = EVENT_EMITTER.get() {
|
||||
emitter.emit(PlayerStatusEvent::ShowNextEpisodePopup {
|
||||
current_episode: current_episode.clone(),
|
||||
next_episode: next_episode.clone(),
|
||||
countdown_seconds,
|
||||
auto_advance,
|
||||
});
|
||||
}
|
||||
|
||||
if auto_advance {
|
||||
// Shared with the frontend-invoked command path
|
||||
// (player_on_playback_ended) so the two dispatchers cannot
|
||||
// disagree about how a background audio-only episode
|
||||
// advances — they did, and the command's copy was missing
|
||||
// the case entirely. That copy is the one that actually
|
||||
// decides here: the end reason set at load makes this
|
||||
// callback's own decision Stop, and the frontend echoes the
|
||||
// resulting PlaybackEnded back into the command.
|
||||
controller
|
||||
.lock()
|
||||
.await
|
||||
.auto_advance_to_next_episode(next_episode, countdown_seconds)
|
||||
.await;
|
||||
}
|
||||
}
|
||||
Ok(AutoplayDecision::ResumeStream { position }) => {
|
||||
// ExoPlayer reported ENDED because the progressive transcode's
|
||||
// connection dropped, not because the episode finished. This
|
||||
// is the arm that matters while backgrounded: it needs no
|
||||
// frontend echo, so the stream re-opens even with the webview
|
||||
// suspended — and playback never parks in STATE_ENDED, where
|
||||
// the next lockscreen/Bluetooth play restarts the item at 0:00.
|
||||
log::info!(
|
||||
"[Autoplay] Decision: Resume truncated stream at {:.1}s",
|
||||
position
|
||||
);
|
||||
let ctrl = controller.lock().await;
|
||||
if let Err(e) = ctrl.resume_stream_at(position).await {
|
||||
log::error!("[Autoplay] Failed to resume truncated stream: {}", e);
|
||||
if let Some(emitter) = EVENT_EMITTER.get() {
|
||||
emitter.emit(PlayerStatusEvent::PlaybackEnded);
|
||||
}
|
||||
}
|
||||
}
|
||||
Err(e) => {
|
||||
log::error!("[Autoplay] Failed to advance to next track: {}", e);
|
||||
log::error!("[Autoplay] Decision failed: {}", e);
|
||||
// Emit PlaybackEnded event on error
|
||||
if let Some(emitter) = EVENT_EMITTER.get() {
|
||||
emitter.emit(PlayerStatusEvent::PlaybackEnded);
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
Ok(AutoplayDecision::ShowNextEpisodePopup {
|
||||
current_episode,
|
||||
next_episode,
|
||||
countdown_seconds,
|
||||
auto_advance,
|
||||
}) => {
|
||||
log::info!(
|
||||
"[Autoplay] Decision: Show next episode popup (countdown: {}s, auto: {})",
|
||||
countdown_seconds,
|
||||
auto_advance
|
||||
);
|
||||
|
||||
// Emit popup event to frontend
|
||||
if let Some(emitter) = EVENT_EMITTER.get() {
|
||||
emitter.emit(PlayerStatusEvent::ShowNextEpisodePopup {
|
||||
current_episode: current_episode.clone(),
|
||||
next_episode: next_episode.clone(),
|
||||
countdown_seconds,
|
||||
auto_advance,
|
||||
});
|
||||
}
|
||||
|
||||
if auto_advance {
|
||||
// Shared with the frontend-invoked command path
|
||||
// (player_on_playback_ended) so the two dispatchers cannot
|
||||
// disagree about how a background audio-only episode
|
||||
// advances — they did, and the command's copy was missing
|
||||
// the case entirely. That copy is the one that actually
|
||||
// decides here: the end reason set at load makes this
|
||||
// callback's own decision Stop, and the frontend echoes the
|
||||
// resulting PlaybackEnded back into the command.
|
||||
controller
|
||||
.lock()
|
||||
.await
|
||||
.auto_advance_to_next_episode(next_episode, countdown_seconds)
|
||||
.await;
|
||||
}
|
||||
}
|
||||
Ok(AutoplayDecision::ResumeStream { position }) => {
|
||||
// ExoPlayer reported ENDED because the progressive transcode's
|
||||
// connection dropped, not because the episode finished. This
|
||||
// is the arm that matters while backgrounded: it needs no
|
||||
// frontend echo, so the stream re-opens even with the webview
|
||||
// suspended — and playback never parks in STATE_ENDED, where
|
||||
// the next lockscreen/Bluetooth play restarts the item at 0:00.
|
||||
log::info!(
|
||||
"[Autoplay] Decision: Resume truncated stream at {:.1}s",
|
||||
position
|
||||
);
|
||||
let ctrl = controller.lock().await;
|
||||
if let Err(e) = ctrl.resume_stream_at(position).await {
|
||||
log::error!("[Autoplay] Failed to resume truncated stream: {}", e);
|
||||
if let Some(emitter) = EVENT_EMITTER.get() {
|
||||
emitter.emit(PlayerStatusEvent::PlaybackEnded);
|
||||
}
|
||||
}
|
||||
}
|
||||
Err(e) => {
|
||||
log::error!("[Autoplay] Decision failed: {}", e);
|
||||
// Emit PlaybackEnded event on error
|
||||
if let Some(emitter) = EVENT_EMITTER.get() {
|
||||
emitter.emit(PlayerStatusEvent::PlaybackEnded);
|
||||
}
|
||||
});
|
||||
} else {
|
||||
log::warn!("[Autoplay] PlayerController not initialized - emitting PlaybackEnded");
|
||||
// Fallback: just emit PlaybackEnded event
|
||||
if let Some(emitter) = EVENT_EMITTER.get() {
|
||||
emitter.emit(PlayerStatusEvent::PlaybackEnded);
|
||||
}
|
||||
}
|
||||
});
|
||||
} else {
|
||||
log::warn!("[Autoplay] PlayerController not initialized - emitting PlaybackEnded");
|
||||
// Fallback: just emit PlaybackEnded event
|
||||
if let Some(emitter) = EVENT_EMITTER.get() {
|
||||
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,
|
||||
) {
|
||||
if let Some(emitter) = EVENT_EMITTER.get() {
|
||||
emitter.emit(PlayerStatusEvent::Buffering {
|
||||
percent: percent as u8,
|
||||
});
|
||||
}
|
||||
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,67 +1064,72 @@ pub extern "system" fn Java_com_dtourolle_jellytau_player_JellyTauPlayer_nativeO
|
||||
message: JString,
|
||||
recoverable: jboolean,
|
||||
) {
|
||||
let message_str: String = env
|
||||
.get_string(&message)
|
||||
.map(|s| s.into())
|
||||
.unwrap_or_else(|_| "Unknown error".to_string());
|
||||
let recoverable = recoverable != 0;
|
||||
jni_guard(
|
||||
"Java_com_dtourolle_jellytau_player_JellyTauPlayer_nativeOnError",
|
||||
|| {
|
||||
let message_str: String = env
|
||||
.get_string(&message)
|
||||
.map(|s| s.into())
|
||||
.unwrap_or_else(|_| "Unknown error".to_string());
|
||||
let recoverable = recoverable != 0;
|
||||
|
||||
// A background audio-only handoff is an mp3 the device was already decoding,
|
||||
// so a recoverable failure part-way through is the network. Surfacing it as a
|
||||
// player error stops playback for good (the frontend's handler calls
|
||||
// player_stop); re-opening the stream where it died is the "buffer and
|
||||
// resume" this actually is. Everything else keeps reporting the error.
|
||||
if recoverable {
|
||||
if let Some(controller) = PLAYER_CONTROLLER.get() {
|
||||
let controller = controller.clone();
|
||||
let message_str = message_str.clone();
|
||||
tauri::async_runtime::spawn(async move {
|
||||
let resume = controller.lock().await.recoverable_error_resume();
|
||||
let Some((position, delay_secs)) = resume else {
|
||||
// Declined here, so report it as NOT recoverable: the frontend
|
||||
// would otherwise echo it into player_recover_stream and ask
|
||||
// the same question a second time.
|
||||
if let Some(emitter) = EVENT_EMITTER.get() {
|
||||
emitter.emit(PlayerStatusEvent::Error {
|
||||
message: message_str,
|
||||
recoverable: false,
|
||||
});
|
||||
}
|
||||
// A background audio-only handoff is an mp3 the device was already decoding,
|
||||
// so a recoverable failure part-way through is the network. Surfacing it as a
|
||||
// player error stops playback for good (the frontend's handler calls
|
||||
// player_stop); re-opening the stream where it died is the "buffer and
|
||||
// resume" this actually is. Everything else keeps reporting the error.
|
||||
if recoverable {
|
||||
if let Some(controller) = PLAYER_CONTROLLER.get() {
|
||||
let controller = controller.clone();
|
||||
let message_str = message_str.clone();
|
||||
tauri::async_runtime::spawn(async move {
|
||||
let resume = controller.lock().await.recoverable_error_resume();
|
||||
let Some((position, delay_secs)) = resume else {
|
||||
// Declined here, so report it as NOT recoverable: the frontend
|
||||
// would otherwise echo it into player_recover_stream and ask
|
||||
// the same question a second time.
|
||||
if let Some(emitter) = EVENT_EMITTER.get() {
|
||||
emitter.emit(PlayerStatusEvent::Error {
|
||||
message: message_str,
|
||||
recoverable: false,
|
||||
});
|
||||
}
|
||||
return;
|
||||
};
|
||||
|
||||
log::warn!(
|
||||
"[ExoPlayer] Recoverable stream error ({}) — re-opening at {:.1}s in {}s",
|
||||
message_str,
|
||||
position,
|
||||
delay_secs
|
||||
);
|
||||
// Give a brief outage time to clear before asking the server for
|
||||
// the stream again; retrying instantly just burns the budget.
|
||||
tokio::time::sleep(std::time::Duration::from_secs(delay_secs)).await;
|
||||
|
||||
let ctrl = controller.lock().await;
|
||||
if let Err(e) = ctrl.resume_stream_at(position).await {
|
||||
log::error!("[ExoPlayer] Failed to resume after error: {}", e);
|
||||
if let Some(emitter) = EVENT_EMITTER.get() {
|
||||
emitter.emit(PlayerStatusEvent::Error {
|
||||
message: message_str,
|
||||
recoverable: false,
|
||||
});
|
||||
}
|
||||
}
|
||||
});
|
||||
return;
|
||||
};
|
||||
|
||||
log::warn!(
|
||||
"[ExoPlayer] Recoverable stream error ({}) — re-opening at {:.1}s in {}s",
|
||||
message_str,
|
||||
position,
|
||||
delay_secs
|
||||
);
|
||||
// Give a brief outage time to clear before asking the server for
|
||||
// the stream again; retrying instantly just burns the budget.
|
||||
tokio::time::sleep(std::time::Duration::from_secs(delay_secs)).await;
|
||||
|
||||
let ctrl = controller.lock().await;
|
||||
if let Err(e) = ctrl.resume_stream_at(position).await {
|
||||
log::error!("[ExoPlayer] Failed to resume after error: {}", e);
|
||||
if let Some(emitter) = EVENT_EMITTER.get() {
|
||||
emitter.emit(PlayerStatusEvent::Error {
|
||||
message: message_str,
|
||||
recoverable: false,
|
||||
});
|
||||
}
|
||||
}
|
||||
});
|
||||
return;
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
if let Some(emitter) = EVENT_EMITTER.get() {
|
||||
emitter.emit(PlayerStatusEvent::Error {
|
||||
message: message_str,
|
||||
recoverable,
|
||||
});
|
||||
}
|
||||
if let Some(emitter) = EVENT_EMITTER.get() {
|
||||
emitter.emit(PlayerStatusEvent::Error {
|
||||
message: message_str,
|
||||
recoverable,
|
||||
});
|
||||
}
|
||||
},
|
||||
);
|
||||
}
|
||||
|
||||
/// Called when volume changes.
|
||||
@@ -1094,16 +1140,21 @@ pub extern "system" fn Java_com_dtourolle_jellytau_player_JellyTauPlayer_nativeO
|
||||
volume: jfloat,
|
||||
muted: jboolean,
|
||||
) {
|
||||
if let Some(state) = SHARED_STATE.get() {
|
||||
state.lock_safe().volume = volume;
|
||||
}
|
||||
jni_guard(
|
||||
"Java_com_dtourolle_jellytau_player_JellyTauPlayer_nativeOnVolumeChanged",
|
||||
|| {
|
||||
if let Some(state) = SHARED_STATE.get() {
|
||||
state.lock_safe().volume = volume;
|
||||
}
|
||||
|
||||
if let Some(emitter) = EVENT_EMITTER.get() {
|
||||
emitter.emit(PlayerStatusEvent::VolumeChanged {
|
||||
volume,
|
||||
muted: muted != 0,
|
||||
});
|
||||
}
|
||||
if let Some(emitter) = EVENT_EMITTER.get() {
|
||||
emitter.emit(PlayerStatusEvent::VolumeChanged {
|
||||
volume,
|
||||
muted: muted != 0,
|
||||
});
|
||||
}
|
||||
},
|
||||
);
|
||||
}
|
||||
|
||||
// JNI callback for MediaSession commands from JellyTauPlaybackService
|
||||
@@ -1127,14 +1178,19 @@ pub extern "system" fn Java_com_dtourolle_jellytau_player_JellyTauPlaybackServic
|
||||
_class: JClass,
|
||||
command: JString,
|
||||
) {
|
||||
let command_str: String = env
|
||||
.get_string(&command)
|
||||
.map(|s| s.into())
|
||||
.unwrap_or_default();
|
||||
jni_guard(
|
||||
"Java_com_dtourolle_jellytau_player_JellyTauPlaybackService_nativeOnMediaCommand",
|
||||
|| {
|
||||
let command_str: String = env
|
||||
.get_string(&command)
|
||||
.map(|s| s.into())
|
||||
.unwrap_or_default();
|
||||
|
||||
if let Some(handler) = MEDIA_COMMAND_HANDLER.get() {
|
||||
handler.on_command(&command_str);
|
||||
}
|
||||
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,14 +1204,19 @@ pub extern "system" fn Java_com_dtourolle_jellytau_player_JellyTauPlaybackServic
|
||||
command: JString,
|
||||
volume: jint,
|
||||
) {
|
||||
let command_str: String = env
|
||||
.get_string(&command)
|
||||
.map(|s| s.into())
|
||||
.unwrap_or_default();
|
||||
jni_guard(
|
||||
"Java_com_dtourolle_jellytau_player_JellyTauPlaybackService_nativeOnRemoteVolumeChange",
|
||||
|| {
|
||||
let command_str: String = env
|
||||
.get_string(&command)
|
||||
.map(|s| s.into())
|
||||
.unwrap_or_default();
|
||||
|
||||
if let Some(handler) = REMOTE_VOLUME_HANDLER.get() {
|
||||
handler.on_remote_volume_change(&command_str, volume as i32);
|
||||
}
|
||||
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,40 +1231,45 @@ pub extern "system" fn Java_com_dtourolle_jellytau_player_JellyTauPlayer_00024Co
|
||||
audio_codecs: JString,
|
||||
max_audio_channels: jint,
|
||||
) {
|
||||
let video_str: String = env
|
||||
.get_string(&video_codecs)
|
||||
.map(|s| s.into())
|
||||
.unwrap_or_default();
|
||||
jni_guard(
|
||||
"Java_com_dtourolle_jellytau_player_JellyTauPlayer_00024Companion_nativeOnCodecsDetected",
|
||||
|| {
|
||||
let video_str: String = env
|
||||
.get_string(&video_codecs)
|
||||
.map(|s| s.into())
|
||||
.unwrap_or_default();
|
||||
|
||||
let audio_str: String = env
|
||||
.get_string(&audio_codecs)
|
||||
.map(|s| s.into())
|
||||
.unwrap_or_default();
|
||||
let audio_str: String = env
|
||||
.get_string(&audio_codecs)
|
||||
.map(|s| s.into())
|
||||
.unwrap_or_default();
|
||||
|
||||
// Kotlin sends 0 when AudioCapabilities had no answer for the current route.
|
||||
let channels = u32::try_from(max_audio_channels).ok().filter(|c| *c > 0);
|
||||
// Kotlin sends 0 when AudioCapabilities had no answer for the current route.
|
||||
let channels = u32::try_from(max_audio_channels).ok().filter(|c| *c > 0);
|
||||
|
||||
let codecs = DetectedCodecs::from_jni_strings(&video_str, &audio_str, channels);
|
||||
let codecs = DetectedCodecs::from_jni_strings(&video_str, &audio_str, channels);
|
||||
|
||||
log::info!(
|
||||
"[CodecDetection] Detected {} video codecs: {}",
|
||||
codecs.video_codecs.len(),
|
||||
codecs.video_codecs_string()
|
||||
log::info!(
|
||||
"[CodecDetection] Detected {} video codecs: {}",
|
||||
codecs.video_codecs.len(),
|
||||
codecs.video_codecs_string()
|
||||
);
|
||||
log::info!(
|
||||
"[CodecDetection] Detected {} audio codecs: {}",
|
||||
codecs.audio_codecs.len(),
|
||||
codecs.audio_codecs_string()
|
||||
);
|
||||
log::info!(
|
||||
"[CodecDetection] Audio route max channels: {:?}",
|
||||
codecs.max_audio_channels
|
||||
);
|
||||
|
||||
// Store in global state
|
||||
if DETECTED_CODECS.set(codecs).is_err() {
|
||||
log::error!("[CodecDetection] Failed to store codecs - already initialized");
|
||||
}
|
||||
},
|
||||
);
|
||||
log::info!(
|
||||
"[CodecDetection] Detected {} audio codecs: {}",
|
||||
codecs.audio_codecs.len(),
|
||||
codecs.audio_codecs_string()
|
||||
);
|
||||
log::info!(
|
||||
"[CodecDetection] Audio route max channels: {:?}",
|
||||
codecs.max_audio_channels
|
||||
);
|
||||
|
||||
// Store in global state
|
||||
if DETECTED_CODECS.set(codecs).is_err() {
|
||||
log::error!("[CodecDetection] Failed to store codecs - already initialized");
|
||||
}
|
||||
}
|
||||
|
||||
/// Start the JellyTauPlaybackService if not already running.
|
||||
|
||||
@@ -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 ")
|
||||
);
|
||||
}
|
||||
}
|
||||
@@ -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"
|
||||
);
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -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;
|
||||
|
||||
@@ -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>>>,
|
||||
|
||||
@@ -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
|
||||
OR l.collection_type NOT IN ('music', 'movies', 'tvshows')
|
||||
)";
|
||||
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
|
||||
);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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
@@ -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();
|
||||
|
||||
@@ -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,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",
|
||||
|
||||
Reference in New Issue
Block a user