diff --git a/docs/codebase-audit.md b/docs/codebase-audit.md index 3833f314..323949d3 100644 --- a/docs/codebase-audit.md +++ b/docs/codebase-audit.md @@ -12,8 +12,8 @@ nothing here is inferred from documentation alone. | Severity | Count | |----------|-------| | High | 5 | -| Medium | 10 | -| Low | 5 | +| Medium | 9 | +| Low | 6 | | Tests passing | 1,719 | | Untraced requirements | 86 | | Traceability coverage | 86% (285/330) | @@ -26,6 +26,11 @@ nothing here is inferred from documentation alone. > backwards: at targetSdk 36 it is already enabled, not merely un-opted-into. > - **B8 added (Medium).** Android 16 Local Network Protections versus a > LAN-hosted Jellyfin server. +> - **D3 Medium → Low.** The "820 unwraps" figure was a measurement error; the +> real number is 19, and none are in command handlers. +> - **B1's stated mechanism was wrong** even though its conclusion held. FGS +> notifications are *not* exempt from `POST_NOTIFICATIONS`; media-session +> notifications are. See B1 — the distinction changes what the fix should be. > > Original ranking was 6 High / 8 Medium / 5 Low. @@ -145,14 +150,25 @@ ServiceRecord{... com.dtourolle.jellytau/.player.JellyTauPlaybackService} category=transport actions=3 vis=PUBLIC) ``` -Foreground-service notifications are exempt from `POST_NOTIFICATIONS` — a -foreground service cannot run without one. All three transport actions are -present. **`UR-006` is not at risk.** +**`UR-006` is not at risk.** But the *reason* is not the one this audit first +gave, and the correction is load-bearing rather than pedantic. -What remains is minor. The FGS notification is the only one the app ever posts: -the single `notificationManager.notify(NOTIFICATION_ID, …)` call updates that same -foreground notification, so it inherits the exemption while the service is -foreground. The declared permission therefore currently buys nothing. +The first explanation here was "foreground-service notifications are exempt." That +is wrong. Android's own wording is that the permission covers "non-exempt +(**including Foreground Services (FGS)**) notifications", and that users who deny +it see FGS notices "in the Task Manager but [not] in the notification drawer" — an +FGS notification is explicitly *not* exempt. What is exempt is **media-session** +notifications. The platform predicate is `Notification.isMediaNotification()`, +requiring `MediaStyle`/`DecoratedMediaCustomViewStyle` **and** a non-null +`EXTRA_MEDIA_SESSION`; it is byte-identical across API 33–36, and +`NotificationManagerService` has no FGS clause in either enforcement site. + +Why the difference matters: under the FGS theory, anything the service posts is +safe, and the code needs no care. Under the correct one, the exemption is earned +per-notification by the token — so losing the token loses not just the shade entry +but the lockscreen controls entirely, since SystemUI's media carousel +(`MediaDataProcessor.onNotificationAdded`) gates on the *same* predicate. A +token-less notification never even reaches the notification listener. **The real risk here is not the permission — it is how narrowly the exemption is earned.** AOSP's `Notification.isMediaNotification()` grants it only when the @@ -401,20 +417,39 @@ subscribers were never cleared, so every module instance discarded by **Location:** `src/lib/services/offlineCatalog.test.ts:58` -### D3 · Medium · 820 `unwrap()`/`expect()` calls sit outside test code +### D3 · Low · ~~820~~ **19** production `unwrap()`/`expect()` calls -They cluster in exactly the files that have historically produced the worst bugs: -`player/mod.rs` (145), `repository/offline.rs` (125), `storage/mod.rs` (70), -`commands/download/mod.rs` (51). A panic inside a Tauri command kills the task and -can leave shared player state inconsistent. +*Downgraded from Medium. This audit substantially overstated the problem, and the +correction is worth recording because the measurement error is instructive.* -Related: 33 raw `.lock().unwrap()` / `.read().unwrap()` / `.write().unwrap()` -calls remain despite the project's own `MutexSafe`/`RwLockSafe` convention, so -poison recovery is not uniform. +The original 820 figure came from grepping for `unwrap()`/`expect()` and filtering +lines containing "test". That does not exclude test *modules* — it only excludes +lines with "test" in them. Scripting the actual `#[cfg(test)]` boundaries gives +**19 real production sites**, not 820. `player/mod.rs`'s 154 hits, for instance, +are *all* past its `#[cfg(test)]` at line 2183, as are the bulk of +`repository/offline.rs`, `storage/mod.rs` and `commands/download/mod.rs`. -**Fix:** Sweep the command-handler paths first, since those are the ones with a -`Result` to return into. Convert the 33 raw locks to the safe helpers -as a mechanical pass. +**More importantly: zero bare unwraps exist in any `#[tauri::command]` handler.** +The specific risk this finding was built around — a panic inside a command killing +the task and stranding shared player state — is already absent. + +The same correction applies to the lock half: all 33 raw `.lock().unwrap()` hits +were in test modules (three weren't even code, but prose in `utils/lock.rs`'s doc +comment). Production was already fully on `lock_safe()`/`read_safe()`/ +`write_safe()`. Converting them was consistency work, not a bug fix. + +**What is genuinely worth doing** is a three-site cluster, all the same pattern — +`Runtime::new().unwrap()` in threads owning playback-critical state: + +| | Site | Consequence of a panic | +|---|------|------------------------| +| 1 | `session_poller/mod.rs:102` | Poller thread dies silently; it drives remote-mode state *and* offline→online recovery, so the app strands offline with nothing surfaced | +| 2 | `player/mpv_backend.rs:424` | Position reporting stops mid-playback; the scrubber freezes while audio keeps going | +| 3 | `player/android/mod.rs:761` | Same pattern across a JNI boundary; progress reporting dies and no resume points are written | + +**Fix:** One shared helper returning `Option` and logging on failure +retires all three. The remaining 16 are startup `expect()`s and two provably +infallible calls. ### D4 · Low · Five files carry a disproportionate share of the complexity diff --git a/src/lib/api/bindings.ts b/src/lib/api/bindings.ts index 1fb105ea..43231cf2 100644 --- a/src/lib/api/bindings.ts +++ b/src/lib/api/bindings.ts @@ -118,16 +118,41 @@ async playerSetVolume(volume: number) : Promise { async playerToggleMute() : Promise { return await TAURI_INVOKE("player_toggle_mute"); }, +/** + * Set the active audio track on a native backend directly. + * + * TRACES: UR-021 | IR-019, DR-024 + */ async playerSetAudioTrack(streamIndex: number) : Promise { return await TAURI_INVOKE("player_set_audio_track", { streamIndex }); }, /** * Switch audio track - handles both HTML5 (stream reload) and native (direct switch) * Note: Frontend should handle saving series preferences after this command succeeds + * + * The split is the requirement: an HTML5 `