Repository navigation
2.1.5 bug fixes: reconnect, offline playback, downloads - #52
Conversation
The connected rising edge only flushed queued progress, so after an outage the library and its "Last sync failed" banner stayed stale until a manual Retry or the next 5 minute tick. A returning server now resyncs whenever the last recorded attempt failed or was partial. SYNCING counts as live so a sync's own status flip never retriggers it. A background sync that brings data back now re-queries the shelf instead of only clearing the banner. Library loads probe /ping before the remote fetch. A live Tailscale interface kept isOnline true with the server gone, so an offline cold start sat on a spinner for the 30s connect timeout before showing saved books. When the connection drops and the auto downloaded-only filter leaves nothing on screen, the empty state now says "Nothing Downloaded" instead of blaming filters, and the banner stops claiming saved books are showing. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The download engine rewrites its own row as Downloading about ten times a second. Pause wrote Paused and only then stopped the worker, and cancel deleted the row first, so one late progress tick undid either of them: a cancelled download came back, a paused one kept going. Delete could have the finishing engine mark the book downloaded again after its files were gone. All three now do what entitlement changes already did. When the book is actively downloading, stop the worker, wait until it has really stopped, then write, then restart the queue for anything else. Pause re-reads the row after the stop so it keeps the latest progress, and leaves a book that finished in that window alone. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The HTTP client is built before settings load, so it always saw the toggle as off and never installed the self-signed handling. Turning it on only saved the preference. Servers with a self-signed certificate could not connect no matter what the switch said. The trust check and hostname check are now always on the client and read the toggle on every connection. Off, they hand the whole decision to the platform trust store and OkHttp's default hostname check, the same check the client made before. On, the existing trust-on-first-use behavior and host match apply, unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
When a download finished while its detail screen was open, the screen flipped to Downloaded but kept the book it loaded before the download, which has no local files. Pressing Play then streamed from the server, or with no connection refused with "You are offline", until you left and came back. The screen now reloads the saved book when its download completes, the same reload it does on returning to the screen. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Pressing Play on a downloaded book first opened a listening session on the server, with the normal 30 second connect and 60 second read timeouts. With the server gone that meant a spinner for up to a minute or more before a book sitting on the phone started playing. A downloaded book now skips that step when the app already knows the server is unreachable, and waits at most 5 seconds for it otherwise. The session is still opened, in the background once playback starts, which is the path restored playback already uses. Streaming books are unchanged. One behavior change: when the session is skipped or late, the start position comes from synced progress (the book's saved position and local progress) instead of the position the server session reports. Those normally agree, since progress sync keeps them in step. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
downloadCompleted replays its last event to new collectors. The book page trusted that event and forced the Downloaded state, so reopening a book whose download had just been deleted showed Downloaded with no way to re-download it until the app restarted. The page now reloads the stored book on a completion event and lets that row decide. This was live in 2.1.4. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
With the server down but a VPN keeping the phone "online", stopping one book and starting a downloaded one sat for the full 30 second connect timeout. The next load waits on the old book's final progress write, and that write was trying the server before letting go. The final position still lands on the phone first, along with its offline queue row. The server push now runs only while the server is known to be live, and gives up after 5 seconds. Either way the queued row goes out on the next reconnect or periodic flush. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Pausing, cancelling or deleting the book that is downloading stops the worker first, waits for it, then writes the change and restarts the queue. All of that ran inside the Downloads screen. Leaving the screen in the middle could stop the worker and skip the rest, so the book stayed Downloading and nothing else in the queue moved until the app restarted. The download manager now runs the whole action itself. The screen still waits for it, but leaving no longer cuts it short. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A download marks itself finished a moment before the book is marked as on the phone, and saves the cover in between. Pausing in that gap left a download that said finished while the book had no local copy. Nothing would pick it back up, Resume refused it, and offline play never unlocked for that book. Pause now only leaves a finished download alone when the book really is on the phone. Otherwise it is paused like any other, and Resume finishes it without downloading the audio again. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Starting the app offline with no saved library list left nothing selected. When the server came back, the background sync cleared the "Last sync failed" banner, but the screen only re-filtered a shelf that had no library behind it, so it stayed empty until a manual refresh. When a sync brings data back and nothing is selected, or the list of libraries changed, the Library now reloads its libraries and picks one, the same way it does on launch. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
When the server drops, the Library switches to downloaded books, and an empty shelf said "Nothing Downloaded". It said that even when downloads existed but Hide Finished, the In Progress or Completed tab, a series or author pick, or a search was hiding them. That sent people looking for books that were on the phone the whole time. The message now only appears when no other filter is narrowing the shelf. Otherwise the usual "Try adjusting your filters" copy shows. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Stopping a book saves its final position on the phone, then sends the listening session's last update to the server and closes it. The next book waits for all of that. With the server gone mid-listen, those two server calls each sat for the full 30 second connect timeout, even after the position push itself was fixed to give up quickly. Both calls now follow the same rule as the position push: they only run while the server is known to be live, and give up after 5 seconds. The position on the phone is saved first, as before. A session that never got its close stays open on the server until the server expires it, which only affects the server's listening stats, not your progress. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
When a reconnect sync found a changed library list, its reload took the library lane and cancelled any manual refresh in flight. The cancelled refresh skipped clearing its spinner, and the replacement never cleared it either, so pull to refresh kept spinning over a loaded shelf. The reload now owns the same cleanup as refresh. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d2683f0a4b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (shouldSyncOnLibraryLoad( | ||
| isLocalLibrary = selected?.isLocal == true, | ||
| isOnline = connectivityMonitor.isOnline.value, | ||
| ) | ||
| ) && connectivityMonitor.checkServerReachable() |
There was a problem hiding this comment.
When loadAudioBooks is called from loadLibraries, the library-list path has already awaited checkServerReachable() at line 333. If that probe times out while the OS still reports an active network (such as the cited Tailscale/server-down case), isOnline remains true, so this condition immediately runs a second independent 5-second probe before showing cached books. Thus a typical cold start with a cached selected library can still remain loading for roughly 10 seconds rather than the intended single 5-second bound; pass the first probe result into this nested load or avoid retrying after it established SERVER_UNREACHABLE.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 4f1147d. The nested item load now reuses the library-list probe result, so a cold start with the server gone waits on one 5s probe instead of two. Confirmed on device: the saved shelf showed in about 16s, down from about 20s. The remaining wait comes from ReachabilityCheckGate deliberately serializing probes across callers (startup sync, library load, periodic check). That's pinned by ReachabilityCheckGateTest so a post-reconnect check never gets a stale failure. Filed as #53 (show the cached shelf first, refresh behind it) rather than changing the gate in a bug-fix release.
…t two loadLibraries probes /ping before fetching the library list, then its nested item load probed again. With the server gone behind a live VPN, the second 5s probe ran only after the first had already failed, so the saved shelf waited about 10s instead of 5. The item load now reuses the list's probe result when it has one. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4f1147d9da
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| shownLibraryIds: List<String>, | ||
| cachedLibraryIds: List<String>, | ||
| ): Boolean = (selectedLibrary == null && cachedLibraryIds.isNotEmpty()) || | ||
| cachedLibraryIds != shownLibraryIds |
There was a problem hiding this comment.
When a server library is removed but retained because it contains downloads, refreshRemoteLibraryList displays it after the fetched libraries, while LibraryDao.getAudiobookshelf() returns all rows by DisplayOrder; if the retained library's old display order places it earlier, these lists contain the same IDs in different orders. This condition then treats every successful sync as a library change, launches loadLibrariesOwningRefresh(), and that load persists another sync sequence which triggers the same comparison again, causing an unbounded cycle of probes and full library reloads. Compare normalized ID sets (or apply the same ordering to both lists) so ordering alone cannot retrigger synchronization.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in e872aee. Membership is now compared as sets, and a test pins that an order-only difference refilters instead of reloading (the canary fails with the ordered compare). I also checked the rest of the reload path for other self-retrigger loops (null selection, cache filtering, the reload's own sync record) and found none.
After a sync, the Library compared its shown library IDs with the cached ones as ordered lists. A removed server library kept for its downloads is shown after the fetched ones, while the cache sorts by display order, so the same libraries read as a change. Every reload wrote a sync record that triggered the next reload. Membership is now compared as a set. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e872aeeec8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| override fun verify(hostname: String, session: SSLSession): Boolean = | ||
| if (allowSelfSigned()) tofu.verify(hostname, session) else platform.verify(hostname, session) |
There was a problem hiding this comment.
When self-signed mode is enabled and Audiobookshelf returns an absolute playback URL or redirect on another HTTPS host—a case explicitly supported by ApiService's contentUrl handling—the trust manager applies the configured server's TOFU fingerprint to that host and this verifier returns false instead of using platform validation. Consequently, valid CDN or proxy-hosted audio cannot play while the toggle is enabled; restrict TOFU to the configured origin and delegate all other hosts to the platform trust manager and verifier.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in df5a2ea. TOFU now applies only when the peer host is the configured server. Every other host goes through the platform trust manager and OkHostnameVerifier, including absolute playback URLs and redirects. Tests cover engine peer-host routing and verifier routing.
| ) | ||
| // Bounded inside deliver, whose own NonCancellable would | ||
| // otherwise outlast any timeout placed around it. | ||
| terminalServerCall(serverLive = true) { |
There was a problem hiding this comment.
When the server becomes blackholed after the last reachability probe, connectionStatus remains CONNECTED, so switching books first spends five seconds in the final progress push, then this session-sync call gets a fresh five-second timeout, and closeSession gets a third one. Because the next load waits for this terminal job, the supposedly bounded transition can still block for roughly 15 seconds; use one shared deadline for the complete terminal sequence or mark the server unavailable after the first timeout.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Declining for 2.1.5, filed as #54. The worst case here is three 5s caps in a row (about 15s), only when the server dies inside the 60s ping window. On master the same switch measured 30s on device, with no bound at all. A shared deadline for the whole terminal job is a refactor, which doesn't fit a bug-fix release.
With the self-signed toggle on, every HTTPS host went through the configured server's trust-on-first-use fingerprint and host check. An absolute playback URL or redirect to another host (a CDN or proxy) was refused even with a valid certificate. TOFU now applies only when the peer host is the configured server. Every other host gets the platform trust manager and hostname verifier, so a self-signed certificate anywhere else is still refused. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: df5a2eaaa8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| isLocalLibrary = false, | ||
| isOnline = connectivityMonitor.isOnline.value, | ||
| ) | ||
| ) && connectivityMonitor.checkServerReachable().also { serverReachable = it } |
There was a problem hiding this comment.
On a cold start with an active network but a blackholed server, NineLivesApp has already called startMonitoring(), which launches its own 5-second reachability check; this new direct call queues behind it because ReachabilityCheckGate only serializes callers and then invokes the checker again. The nested item-load probe from the earlier comment is now reused, but the fresh evidence is ReachabilityCheckGateTest, which confirms two concurrent calls still execute two checks sequentially, so cached books can remain behind the spinner for roughly 10 seconds. Reuse/coalesce the in-flight startup probe rather than starting another one here.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Declining for 2.1.5, tracked in #53. This is the deliberate probe serialization in ReachabilityCheckGate. It's pinned so a check started after reconnect never gets a stale failure, and changing that is out of scope for a bug-fix release. It isn't a regression: the same cold start measured 30 to 47s on master and about 16s on this branch. #53 takes the probe off the first-paint path (show the cached shelf first, refresh behind it).
1 participant
Bug-fix-only candidate for 2.1.5 from the post-relaunch review. No new features, no version bump yet (that follows on Jeff's go).
What users hit, and what changes
Server outages (the one Jeff reported)
52dceae). A background sync that brings data back re-queries the shelf, and reloads the library list when nothing is selected or the list changed (9400ef7, spinner race fixed ind2683f0)./ping(5s) first.e603e5e).Offline playback
519417f).3b26664,78000a4).1663e14). It also ignores the replayed completion event, which on 2.1.4 left a deleted download showing "Downloaded" (906b7e3).Downloads
6bd77e5), run in a manager-owned scope so leaving the screen can't abandon them (f2d491b), and pause near completion no longer strands a half-finalized row (d4177f7).Self-signed certificates
5ad6b3d).Review
Device evidence (SM-S948U, debug)
Not device-run: the self-signed toggle (unit-tested against embedded certs) and the
d2683f0spinner race.Also filed: #51 (cancelled downloads leave partial files, pre-existing).
🤖 Generated with Claude Code