fix(playback): make session ownership survive cancellation on both clients - #178
Merged
Merged
Conversation
…ients Every defect here is the same shape: something proves it owns a playback session, suspends, and never re-checks — or hands ownership on without moving the token that names it. The result is a transcode nobody remembers, holding a server stream slot until it expires, or a teardown that stops the wrong session and lets the right one run on. PlaybackSessionManager - Lease every server-allocated session from the moment the start response decodes. Each branch of startVideoSessionV3 still suspends before it publishes that id or stops it, and a cancellation in between left the id owned by nobody: the manager never published it, and callers only learn it when the function returns. The lease clears only where responsibility genuinely moves — publication, or a stop the server acknowledged — and the finally releases whatever is still held, uncancellably. - Give the internal replan the same discipline. Those paths clear activeVideoAttempt or remove the staged handle before their own suspending stop, and they run from ViewModel recovery jobs that exit and content replacement cancel. stopRetainingFailureLocked registers the id under the caller's lock and queues the stop; drainPendingOwnershipReleases issues it once the lock is released, still awaited by the caller that queued it. Awaiting a 60s-timeout request under videoAttemptMutex would have serialised every start, replan and content reset behind a dying session's teardown. - Tag queued releases with a claim, so one caller cannot take another's work — returning before its own stop ran while an unrelated caller blocks on someone else's teardown. - Count in-flight releases rather than flagging them, and exclude queued and in-flight ids from the orphan drain, so two paths cannot stop one session concurrently. - Bound the orphan ledger. Only a discharged stop removes an entry, so a server that keeps failing this call while playback produces new sessions grew it without limit and made every content reset retry a larger set. - Treat a typed session-missing 404 as discharged; a bare 404 proves nothing about the session and kept the id queued forever. - abandonActiveVideoSessionIfCurrent: the unconditional variant stops even when it failed to disown, and stopSession's predecessor branch then clears a newer pending publication and stops its replacement. Release on a dispatched scope widened that window enough to matter. PlaybackSessionLifecycle - Restore the ownership token totally on rollback. Assigning it only when the snapshot was Active left a rolled-back first deferred adoption naming the discarded replacement. Phone and TV ViewModels - Put phone's exit and onCleared behind the one-shot PlaybackTeardownGate. Naming the session is necessary but not sufficient: the second stop passes the ownership guard because the first cleared the owner, and bumps stopEpoch on its way through, superseding a screen that has acquired its start epoch but not yet adopted. TV has been behind this gate since auto-advance broke on exactly that race. - Read the retained token before UI state on both clients. Every adoption path takes lifecycle ownership before it publishes, so reading UI first inside that gap named the predecessor, the lifecycle rightly refused, and the gate stopped onCleared retrying. The token now moves with ownership in both directions — forward at each adoption and at load publication, back to the predecessor on either TV rollback path. - Port TV's replan abandonment protocol to phone, and give both clients conditional adoption with cancellation-safe candidate cleanup: a cancellation while awaiting the lifecycle mutex throws before isCurrent runs, and runCatching in an already-cancelled coroutine releases nothing. - Seek recovery deliberately does NOT stop on refusal, on either client. seekReanchorMismatch rejects any response whose session id differs from the active attempt, so a re-anchor always reuses the base session — the id names the session still playing, and refusal normally means a newer seek was queued that needs it as its base. - Guard TV's onCleared teardown with a non-null session id. A null expectedSessionId disables the ownership guard entirely, and that callback is delayed behind subtitle settlement. - Release abandoned sessions on the manager's own scope rather than viewModelScope.launch(NonCancellable), which severs structured concurrency to produce a coroutine nothing can await or observe. Reviewed file-by-file with Codex over seven passes; two of these defects were introduced by earlier attempts at the others. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0115uaQ6FTQZK8KYvazjefaW
Follow-up to 104aacc, correcting a regression that commit introduced and two claims its message made that the code did not support. PlaybackSessionLifecycle - 104aacc made restoreActiveSessionSnapshot assign lastAdoptedSessionId "totally", deriving it from the restored state. That fixed one end and broke the other. SessionState carries a session id only while Active, but Reconnecting and Failed deliberately keep owning theirs — that is the whole reason the token exists separately from the state. Deriving therefore erased ownership for exactly the states meant to survive an outage: a rollback restoring a predecessor as Reconnecting cleared its token, and stop() then found no id to name, leaving that transcode to run until the server expired it. The snapshot now captures the token alongside the state and restores what it captured, which is correct at both ends. PlaybackSessionManager - Re-trim the orphan ledger when the drain path clears its in-flight marker, not only when a queued release completes. Trimming skips in-flight ids, so whichever path clears the last marker has to re-apply the cap; otherwise a burst drained from the orphan path left the ledger over its bound until unrelated future activity happened to trim. - Queue a release before registering it as an orphan. Registration trims, and trimming protects only ids already queued or in flight, so with a full ledger of protected entries the session being queued could be the single evictable entry and be dropped before its stop had begun. Corrections to 104aacc's message, which overclaimed: - "restore the ownership token totally" was wrong for the owning non-Active states, as above. - "so two paths cannot stop one session concurrently" was false. The count keeps the in-flight marker honest across an overlap it explicitly supports; it does not prevent the second request. That is tolerable only because stopping an already-stopped session is harmless, and the comment now says so. Found by an eighth Codex pass over the committed work. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0115uaQ6FTQZK8KYvazjefaW
reportPosition wrote process-global fields, and the 10s reporter loop pairs those with whichever session is Active when it next fires. A final callback from an outgoing player — arriving after the next screen has adopted — therefore flushed the previous episode's position under the new episode's id. That is the "resume jumped to the last episode's time" report. The caller now names the session it believes produced the sample, and the lifecycle drops anything that does not match the session it owns. null means "I own no session", NOT "skip the check". That distinction is the fix: both exit paths clear the UI session id while player callbacks are still draining, so treating null as permission would have left the original corruption path wide open. A caller without a session may write only while the lifecycle owns none either — which is exactly downloaded and local playback, whose resume position is persisted separately and does not depend on this path. Making the parameter required broke eleven test call sites, which is the useful kind of breakage: those tests were reporting without saying what they owned. They now go through a helper that names the owned session, the same way the players do. Verified against outage recovery (ownership survives Reconnecting and Failed, so samples keep landing), the adoption-to-publication gap, the exit window, cast, and Watch Together. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0115uaQ6FTQZK8KYvazjefaW
Being installed as the active attempt is not the same as being findable. In the legacy-engine ReplanRequired branch the lease was released the moment the session became the active attempt, but nothing outside this call has the id at that point, and the nested replan then suspends twice — finishContentReset, then its own mutex — before reaching any cleanup of its own. A cancellation in that window left the session installed, unknown to every caller, and running until the server expired it. The lease now stays armed across that call, and the branches after it decide its fate: abandoned ids keep the lease until their stop is acknowledged, and a successful publication clears it because the manager owns the outcome. That last branch is new and load-bearing — without it, holding the lease longer would have made the finally stop a session that is playing, which is worse than the leak it closes. Also narrows the trim comment: it protects ids the release queue knows about, not every stop in flight, since committed cleanup and the direct retaining-stop helpers issue unmarked ones. Found by Codex reviewing the rebased series against current main — the same rebase-time review that has caught the real defects all day. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0115uaQ6FTQZK8KYvazjefaW
|
Warning Review limit reached
Next review available in: 40 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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 (1)
📝 WalkthroughWalkthroughPlayback session ownership now survives lifecycle snapshots, rejects stale position callbacks, and coordinates session cleanup through leases, release claims, bounded orphan tracking, and session-qualified teardown across mobile and TV players. ChangesPlayback session ownership
Estimated code review effort: 5 (Critical) | ~90 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant PlayerViewModel
participant PlaybackSessionLifecycle
participant PlaybackSessionManager
participant PlaybackTeardownGate
participant PlaybackSessionRepository
PlayerViewModel->>PlaybackSessionLifecycle: adopt replacement session
PlaybackSessionLifecycle-->>PlayerViewModel: accept or reject ownership
PlayerViewModel->>PlaybackTeardownGate: request session-qualified teardown
PlaybackTeardownGate->>PlaybackSessionManager: stop retained session
PlaybackSessionManager->>PlaybackSessionRepository: stop or retry session
PlaybackSessionRepository-->>PlaybackSessionManager: success or missing-session response
PlaybackSessionManager-->>PlaybackTeardownGate: discharge or retain orphan
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
android-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/PlaybackSessionManager.kt (2)
1184-1188: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winLog the failure and let
CancellationExceptionpropagate.
runCatchingdiscards everyThrowable, includingCancellationException. Two consequences follow.A failed abandonment leaves the session running until the server expires it, and nothing records that it happened.
stopAsyncinPlaybackSessionLifecyclelogs the equivalent failure withLog.w, andscheduleRegisteredCommittedSessionCleanuphandles cancellation with an explicitcatch. This path is the only one that stays silent.Swallowing
CancellationExceptionalso makes a cancelled job complete normally.sessionCleanupScopeis long-lived in production, so this matters mainly when a test injects a structured scope.♻️ Proposed change
fun abandonActiveVideoSessionAsync(sessionId: String) { sessionCleanupScope.launch { - runCatching { abandonActiveVideoSessionIfCurrent(sessionId) } + try { + abandonActiveVideoSessionIfCurrent(sessionId) + } catch (cancellation: CancellationException) { + throw cancellation + } catch (t: Throwable) { + Log.w(TAG, "async abandon failed for $sessionId", t) + } } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@android-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/PlaybackSessionManager.kt` around lines 1184 - 1188, Update abandonActiveVideoSessionAsync to replace runCatching with explicit exception handling around abandonActiveVideoSessionIfCurrent: log non-cancellation failures with Log.w using the same context as PlaybackSessionLifecycle.stopAsync, and rethrow CancellationException so cancellation propagates through sessionCleanupScope.
1594-1606: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename these helpers to the
Lockedconvention and drop the vestigialsuspendmodifiers.Line 1605 and Line 1624 now call
stopRetainingFailureLocked, which is not a suspending function. After this change neither helper performs suspending work, so thesuspendmodifiers no longer describe them.stopCandidateSessionsIfUnownedinherits the same state.The naming matters more. This file uses a
Lockedsuffix to mark functions that requirevideoAttemptMutex. All three helpers now require it: they callstopRetainingFailureLockedand they readstagedVideoReplansandactiveVideoAttempt. Every current caller holds the mutex, so the code is correct today. The names no longer signal the requirement, andpendingOwnershipReleasesandorphanedSessionIdsare plain collections that a future unlocked caller would corrupt.♻️ Proposed rename
- private suspend fun stopCandidateSessionIfUnowned( + private fun stopCandidateSessionIfUnownedLocked( activeSessionId: String?, candidateSessionId: String?, ) {- private suspend fun stopCandidateSessionsIfUnowned( + private fun stopCandidateSessionsIfUnownedLocked( activeSessionId: String?, vararg candidateSessionIds: String?, ) { candidateSessionIds.filterNotNull().distinct() - .forEach { stopCandidateSessionIfUnowned(activeSessionId, it) } + .forEach { stopCandidateSessionIfUnownedLocked(activeSessionId, it) } }- private suspend fun stopImmediateFailureSessions( + private fun stopImmediateFailureSessionsLocked( activeSessionId: String, candidateSessionIds: List<String?>, stopActiveSession: Boolean, ) { if (stopActiveSession) { stopRetainingFailureLocked(activeSessionId) } candidateSessionIds.filterNotNull().distinct() .filter { it != activeSessionId } - .forEach { stopCandidateSessionIfUnowned(activeSessionId = null, it) } + .forEach { stopCandidateSessionIfUnownedLocked(activeSessionId = null, it) } }Update the call sites at Lines 871, 895, 960, 975, 990, 1000, 1011, 1026, and 1069 accordingly.
Also applies to: 1616-1629
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@android-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/PlaybackSessionManager.kt` around lines 1594 - 1606, Rename stopCandidateSessionIfUnowned and stopCandidateSessionsIfUnowned to their Locked-suffixed forms, and apply the same Locked naming convention to the third related helper covering the lines around 1616-1629. Remove suspend from all three helpers, then update every listed call site and any other references to use the new names while preserving their existing mutex-protected behavior.android-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/player/PlaybackSessionLifecycleTest.kt (1)
1025-1041: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd one focused test for the rejection path of
reportPosition.
reportOwnedPositiononly exercises the accept path. Every changed call site reports while the state isActive, so no test proves that a foreign session ID is rejected. That guard is the new behavior of this layer, and a regression would restore the cross-episode progress write.Two cases are worth pinning:
- A sample carrying a stale session ID does not change the reported position.
- A sample carrying
nullis rejected while the lifecycle still owns a session.Note also that the helper derives the ID from
SessionState.Activeonly. While the lifecycle isReconnectingorFailedit passesnull, which production rejects because the token survives those states. No current test calls it in those states.As per coding guidelines: "Add focused tests for shared logic only when behavior is critical or high risk".
💚 Proposed test for the rejection path
`@Test` fun `reportPosition ignores samples from a session the lifecycle does not own`() = runTest { val sessionMgr = FakeSessionManager() val lifecycle = newLifecycle(sessionMgr, scope = backgroundScope) lifecycle.adoptActiveSession(defaultStartParams(), makeSession("sess-current")) lifecycle.reportOwnedPosition(positionSec = 20.0, durationSec = 100.0, isPaused = false) // A late callback from the outgoing player, and a caller claiming no session. lifecycle.reportPosition(90.0, 100.0, isPaused = false, expectedSessionId = "sess-previous") lifecycle.reportPosition(95.0, 100.0, isPaused = false, expectedSessionId = null) advanceTimeBy(PlaybackSessionLifecycle.PROGRESS_REPORT_INTERVAL_MS + 100) assertEquals("sess-current", sessionMgr.lastProgressSessionId) assertEquals(20.0, sessionMgr.lastProgressPosition) lifecycle.stop() }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@android-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/player/PlaybackSessionLifecycleTest.kt` around lines 1025 - 1041, Add a focused test near the existing PlaybackSessionLifecycle tests that adopts an active session, records an accepted position through reportOwnedPosition, then calls reportPosition with a stale session ID and with null. Advance past the progress reporting interval and assert the session manager still contains the owned session ID and original position, then stop the lifecycle.Source: Coding guidelines
androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/player/PlayerViewModel.kt (1)
2859-2861: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winRecord the id of the adopted
ready.session.
ready.session.sessionIdis the id passed tosessionLifecycle.adoptActiveSessionIfCurrent; it matches the session later assigned to UI state. Use that id here instead of nullableplayback.sessionIdsoretainedOwnedSessionIdcannot keep the predecessor’s name while the lifecycle owns the replaced session.♻️ Proposed change
if (lifecycleAdopted) { - playback.sessionId?.let { retainedOwnedSessionId = it } + retainedOwnedSessionId = ready.session.sessionId }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/player/PlayerViewModel.kt` around lines 2859 - 2861, Update the lifecycleAdopted branch in the relevant PlayerViewModel flow to assign retainedOwnedSessionId from the adopted ready.session.sessionId value passed to sessionLifecycle.adoptActiveSessionIfCurrent, rather than nullable playback.sessionId; preserve the existing behavior when adoption does not occur.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/player/TvPlayerViewModel.kt`:
- Around line 4799-4804: Resolve the teardown session ID inside the
post-settlement callback in invalidateAndSettleAsync, using the current
lastAdoptedSessionId after rollback rather than the onCleared() snapshot
teardownSessionId. Pass that resolved ID to lifecycleTeardown.stopDetached only
when it is non-null, so rolled-back subtitle adoption tears down the predecessor
session and preserves the ownership guard.
---
Nitpick comments:
In
`@android-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/PlaybackSessionManager.kt`:
- Around line 1184-1188: Update abandonActiveVideoSessionAsync to replace
runCatching with explicit exception handling around
abandonActiveVideoSessionIfCurrent: log non-cancellation failures with Log.w
using the same context as PlaybackSessionLifecycle.stopAsync, and rethrow
CancellationException so cancellation propagates through sessionCleanupScope.
- Around line 1594-1606: Rename stopCandidateSessionIfUnowned and
stopCandidateSessionsIfUnowned to their Locked-suffixed forms, and apply the
same Locked naming convention to the third related helper covering the lines
around 1616-1629. Remove suspend from all three helpers, then update every
listed call site and any other references to use the new names while preserving
their existing mutex-protected behavior.
In
`@android-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/player/PlaybackSessionLifecycleTest.kt`:
- Around line 1025-1041: Add a focused test near the existing
PlaybackSessionLifecycle tests that adopts an active session, records an
accepted position through reportOwnedPosition, then calls reportPosition with a
stale session ID and with null. Advance past the progress reporting interval and
assert the session manager still contains the owned session ID and original
position, then stop the lifecycle.
In
`@androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/player/PlayerViewModel.kt`:
- Around line 2859-2861: Update the lifecycleAdopted branch in the relevant
PlayerViewModel flow to assign retainedOwnedSessionId from the adopted
ready.session.sessionId value passed to
sessionLifecycle.adoptActiveSessionIfCurrent, rather than nullable
playback.sessionId; preserve the existing behavior when adoption does not occur.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a92c8544-395d-41cd-bd6b-1d54dafdd713
📒 Files selected for processing (7)
android-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/PlaybackSessionLifecycle.ktandroid-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/PlaybackSessionManager.ktandroid-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/player/PlaybackSessionLifecycleTest.ktandroid-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/player/PlaybackSessionManagerStagedReplanTest.ktandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/player/PlayerViewModel.ktandroidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/performance/MobilePlayerLifecyclePerformanceSourceTest.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/player/TvPlayerViewModel.kt
onCleared captured exitSessionId before invalidateAndSettleAsync, whose callback runs once settlement completes. Settlement can roll a subtitle publication back, and that rollback hands ownership to the predecessor — so the captured value named the discarded replacement, the ownership guard refused it, and the one-shot gate was already consumed. The predecessor's session was left running until the server expired it. Reading the token inside the callback gets the owner as it stands after settlement, which is the only point at which it is settled. This is the read-site counterpart to the rollback fix in this series: the token moves back on rollback, so anything snapshotted before settlement is stale by construction. Found by CodeRabbit on Silo-Server#178. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0115uaQ6FTQZK8KYvazjefaW
Contributor
Author
|
Good catch — fixed. You're right that the capture is stale by construction: settlement can roll a subtitle publication back, and that rollback returns ownership to the predecessor, so a value read before the callback names the discarded replacement. The guard then refuses it while the one-shot gate has already been consumed, and the predecessor's session runs on until the server expires it.
This is the read-site counterpart to a fix already in this series: the token is moved back to the predecessor on rollback, so anything snapshotted before settlement was going to be wrong. Full suite green on all four modules. |
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.
Every defect here is the same shape: something proves it owns a playback session, suspends, and never re-checks — or hands ownership on without moving the token that names it. The result is a transcode nobody remembers, holding a server stream slot until it expires, or a teardown that stops the wrong session and lets the right one run on.
PlaybackSessionManager
An allocation lease. After
startPlaybackV3returns a session id, every branch still suspends before it publishes that id or stops it — and a cancellation in between left the id owned by nobody: the manager never published it, and callers only learn it when the function returns. The lease is armed the moment the response decodes and cleared only where responsibility genuinely moves.That includes across the nested legacy-engine replan. Being installed as the active attempt is not the same as being findable: nothing outside the call has the id yet, and the replan suspends twice more before reaching cleanup of its own.
Ownership released without holding the lock. The internal replan paths clear
activeVideoAttemptor remove the staged handle before their own suspending stop, and they run from ViewModel recovery jobs that exit and content replacement cancel. Releases now register under the caller's lock and are issued once it is released — awaiting a 60s-timeout request undervideoAttemptMutexwould serialise every start, replan and content reset behind a dying session's teardown.Each release is tagged with a claim so one caller cannot take another's work, in-flight releases are counted rather than flagged, and the orphan ledger is bounded: only a discharged stop removes an entry, so a server that keeps failing this call grew it without limit.
Lifecycle and both ViewModels
The ownership token is snapshotted rather than derived —
ReconnectingandFaileddeliberately keep owning their session, so reconstructing the token from the restored state erased ownership for exactly the states meant to survive an outage.Phone exit and
onClearedgo behind the one-shot teardown gate; both clients read the retained token ahead of UI state, because every adoption path takes ownership before it publishes; adoption is conditional with cancellation-safe candidate cleanup; and seek recovery deliberately does not stop on refusal, because a re-anchor is validated to reuse the base session — that id names the session still playing.reportPositionnow requires the caller to name its session.nullmeans "I own no session", not "skip the check": both exit paths clear the UI session id while player callbacks are still draining.Review
Nine Codex rounds. The last was against this base after rebasing — which is where the real defects have consistently been — and it found the replan lease boundary above. Fixing that introduced a second hazard in the same change (a successful replan would have had the
finallystop a live session), closed in the same commit.Full suite green on all four modules with
--rerun-tasks. Not device-verified.🤖 Generated with Claude Code
https://claude.ai/code/session_0115uaQ6FTQZK8KYvazjefaW
Summary by CodeRabbit