fix(android): harden player lifecycle and final writes (PR 108 slice D) - #115
Merged
Conversation
start() ran its API call outside the lifecycle mutex, and for that whole window _state is Loading with lastAdoptedSessionId null — so stop()'s ownership guard found no id to compare against and tore down anyway. Two consequences. The teardown nulls lastStartParams, and startInternal never re-set it, so the session that then published Active had none: 404 session recovery and the final progress flush both silently no-op, losing the user's resume position on exit. And the newly started session was never stopped, because the stop that would have owned it had already run — leaving it alive on the server, counting against the account's concurrent-stream cap until it timed out. That is the same shape as the orphaned cast sessions that produced the 429 storm. Publish the start result under the mutex and compare a stop epoch captured before the API call: if a teardown ran while we were starting, stop the new session and stay Idle instead of resurrecting a dismissed screen. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…indow The 90s window was enforced against an estimate reconstructed from the retry count, which sums only the delays between attempts. Time spent inside each failed attempt was invisible, so against a server that accepts connections and then stalls — 30s socket timeout per try — the real elapsed time ran several times the window before an error surfaced, leaving a spinner up for minutes. Measure from the first error of each load task and take whichever bound is larger. Callers that cannot measure keep the derived behaviour. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two findings CodeRabbit raised on PR #97. That PR's code was squashed into main without them being triaged, so both were live here. FinalPlaybackPositionWriter drained `pending` before writing and swallowed the result, so a failed write discarded the user's final position outright — no retry, no re-queue — and this queue is the durable record that survives a server-side session reset. Failures now go back on the queue with a backoff, using putIfAbsent so a newer position submitted meanwhile still wins. PlayerViewModel.onExit did not cancel the Up Next countdown, so a countdown still running when the user left could fire and start an episode behind a dismissed screen — creating exactly the kind of session that outlives the exit meant to end it. Also makes the content-reset self-heal timeout injectable. runTest advances virtual time whenever the scheduler idles, so the fixed 45s value could fire inside tests that legitimately wait for a settlement, which made `new content start waits for unresolved replacement settlement` order-dependent. Tests asserting the wait now pass NEVER_SELF_HEAL; the self-heal test asks for the real value. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
(cherry picked from commit 1e8cb51)
|
Warning Review limit reached
Next review available in: 59 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (30)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stack
split/108-c-room-sync)integrate/ship-everythingremain archival references.Scope
Transactional subtitle publication, PGS/libass, and letterboxing remain in slice E.
Review corrections
Independent review findings were fixed test-first: stale success/failure publication after exit, late external adoption leaks, valid starts racing queued lazy stops, cancellation between server allocation and adoption, and unwired rollup pause/resume. Final independent verdict: Ready: YES; no remaining Important/Critical findings.
Verification
./scripts/test-check-build-supply-chain.sh./scripts/check-build-supply-chain.sh./gradlew -Dorg.gradle.jvmargs="-Xmx4g -Dfile.encoding=UTF-8" testDebugUnitTest :androidApp:assembleRelease :androidTvApp:assembleRelease --max-workers=22e9ad5f2: BUILD SUCCESSFUL, 334 tasks, 2m50sTraceability and deferred-commit mapping:
docs/superpowers/plans/2026-07-27-pr108-slice-d-player-foundation.md.