Repository navigation
fix(player): refresh track menus from deferred inventory pushes - #218
Conversation
Production-readiness review: Request changes (Grade 68/100)Reviewed Blocker 1 — P1: outgoing candidate can veto an exact match to the winning plan
Fix: for a replaced plan with a concrete URI, decide by exact plan identity first. Keep collision checks for cases where the winner cannot resolve candidate identity. Blocker 2 — P1: concrete-source exception still rejects the newer same-file source commit
Fix: under an admissible URI-less winner, replay authoritative concrete source commits in sequence without letting an earlier captured candidate veto the later commit. Preserve inventory collision protection; don't broadly disable it. Blocker 3 — P1: URI-only admission can attach an outgoing inventory to an unrelated replacement file
Fix: require corroboration for URI-only cross-adoption inventory. Preserve admission when the URI ties to the settled source; for ambiguous cross-file replacement, defer to a current-source refresh rather than inheriting the replacement file ID. CIAll 8 checks pass on this head — green CI demonstrates the covered cases but doesn't close the three predicate gaps above. Acceptance: the three regressions above green, predicates fixed, focused tests + gate rerun. No middleware or |
…ushes Reconcile deferred realtime pushes against the adoption that actually won, closing three frontend review blockers. - Decide a replaced plan that names a candidate URI by exact identity before the outgoing/applied ambiguity guards, so an outgoing candidate the session was leaving cannot veto the plan the server just selected. - Carry whether an entry's arrival-time outgoing baseline was itself produced by an earlier deferred source commit from the same queue. A URI-bearing source commit is judged by arrival order, so such a queue-produced baseline no longer vetoes a newer same-file commit; an external poll fold still does. - Require corroboration for a URI-only push against a URI-less winner: admit it only when the arrival-time live source was already on the settled file, so an unrelated replacement file never inherits its candidate. Add regressions for all three; the primary relaxation (same-file candidate-bearing pushes against URI-less plans; refused revisions refoldable) is preserved.
drondeseries
left a comment
drondeseries
left a comment
There was a problem hiding this comment.
Independent review (oracle, final): 74/100 — needs work, not ready to merge.
The change is still needed after #220 (no file overlap; complementary).
Blocker — poll vs deferred-source ordering (web/src/player/hooks/usePlaybackSession.ts:356, :2624, :2675-2682):
A URI-bearing source commit ignores appliedIdentity even when a poll folded a newer candidate after it was queued. Example: outgoing file A, deferred commit selects file B/candidate X, a later poll applies B/candidate Y, the replacement plan names B without a URI. The outgoing baseline is cross-file, so X passes and overwrites Y. Sorting covers order inside the queue, not between the queue and an applied poll. :318-319 and :2669-2671 describe protection the code lacks.
Minimal fix: keep the applied-candidate guard for URI-bearing commits (tell older applied state apart from polls newer than the deferred entry if needed). Add regression tests for both orderings so a stale baseline cannot veto a newer commit.
Non-blocking: fix the stale wording at :318-319 and :1003-1007 (refused revisions are deliberately not recorded), and add the null-URI bogus-candidate test.
Please rebase onto current main and require green CI before merge.
A URI-bearing source commit deferred behind a start/switch/replan ignored the applied candidate entirely, so a poll that folded a newer candidate after the commit was queued could be overwritten once the plan settled. In-queue sequence sorting covers order inside the queue, not between the queue and an applied poll. Judge a URI-bearing commit against its own arrival baseline and the queue's earlier commits by arrival order, but still veto it with a candidate a poll folded after the entry was queued, tracked by a source-identity transition clock. Record the poll's candidate so it survives the winning plan's own adoption overwriting the menu mirror before the flush runs. Also fix stale wording about refused revisions being unrecorded at the admissibility doc and revision-scope comment, and add a null-URI bogus-candidate regression.
|
Review follow-up addressed and pushed on
|
drondeseries
left a comment
drondeseries
left a comment
There was a problem hiding this comment.
Re-review (oracle, final): 93/100 — APPROVED for the targeted fix in 8c83b8d.
Verified present: arrivalTransitionClock on deferred pushes, identityTransitionClockRef + lastPollAppliedRef, appliedPollNewer threaded into the admissibility gate, baselines [outgoing] + poll-newer veto for URI commits. Clock raised by poll folds and plan adoptions; poll identity survives adoption; both orderings covered by regression tests.
The one Go-test failure on this head (TestServeExtractTextWindowDoesNotPoisonFullTrack, internal/playback/subtitle_cache_test.go:151) is unrelated to this PR: the branch touches only the two web session files, and the failing package is untouched. Reads as a main-side flake or ordering issue, not this change — a retry should confirm.
Merging once CI is green on a retry.
|
Approval noted, thanks. Direct rerun of the failed Go-test run was rejected ("workflow file may be broken"), so CI was retriggered via empty commit |
2 participants
Problem
Related issue: N/A
Validation tasks: none
During ongoing playback the subtitle and audio menu lists keep the plan-time snapshot and never pick up the verified tracks; only closing and resuming the player shows the right lists. Follows up the #215 deferred-push work: identity-carrying inventory pushes are refused against URI-less settled plans, and the refused revision is then permanently recorded, so redeliveries die in the revision gate too.
Approach
Admit same-file inventory pushes against URI-less plans while still refusing genuine same-file sibling collisions and file-only pushes under a concrete live candidate; record only revisions actually folded so a refused revision can apply after a later matching rotation. Adds regression tests for the URI-less, file-only, ordering, and redelivery cases.
Validation
Risks
Track menus can now adopt inventory from a same-file push the old code dropped. Sibling collisions and cross-file rotations are still refused. No API, migration, or config impact.
Checklist
AI Disclosure