Repository navigation
fix(playback): bind virtual subtitle evidence to the exact catalog row - #219
Conversation
Duplicate catalog rows for one release share candidate URIs and neutral keys. A serve-layer probe rotation could move a session's binding from the played row A onto sibling row B, and the URI-only evidence match then applied A's audio/subtitle inventory to B and B's to A. The same ambiguity let the version list project one release twice. Record the evidence row id at plan time and require it, together with the candidate URI, when applying carried evidence: - Session/SessionStreamState carry VirtualSubtitleEvidenceFileID, captured from the effective file in v3SessionStreamState and cleared with the rest of the evidence. A zero id (legacy or reconstructed session) keeps the URI-only fallback. - virtualEvidenceMatchesBoundFile and refusedProbeInventoryFile require the row id and URI to match, so the in-memory refused-probe publish reaches only the row that captured the evidence. Stop cross-row contamination on probe rotation: when a probed candidate path is owned by a sibling row of the same release (matching durable provider identity), virtualProbeEvidenceRotateTarget refuses the rotation instead of overlaying the requested row's tracks onto the duplicate sibling, and serves the probed inventory in memory to the requested row's own sessions. A genuinely different release still rotates to its owner row. Collapse duplicate virtual rows in the version list: byte-identical rows collapse to the first occurrence, and an unprobed placeholder collapses when a probed copy of the same neutral release exists. A probed copy is preferred; a placeholder with no probed copy stays listed. Tests prove each behavior; all touched packages pass.
Production-readiness review: Request changes (Grade 62/100)Reviewed Merge blocker — required lint checks fail (fix first)
MERGEABLE means conflict-free, not ready. These block merge independently of the findings below. Blocker 1 — High: version collapse treats "unprobed" as "placeholder"
Fix: require positive placeholder identification or verified duplicate identity; absence of a probe stamp is not sufficient. Blocker 2 — High: outgoing candidate can veto the explicitly winning plan
Fix: resolve replacement-plan authority first; apply ambiguity guards only when the winning plan cannot identify the candidate. Blocker 3 — High: URI-less plans admit pushes with no file identitySame function, final return — admission explicitly accepts Fix: require a matching, non-null file ID for a URI-less winner, unless another explicit source-generation binding proves ownership. Blocker 4 — Medium: known row provenance can bypass the row check
Fix: enforce known evidence-row identity before either URI path; keep URI-only compatibility only for genuinely unknown evidence provenance. Acceptance: lint green, the four fixes with their regressions, refused-sibling rotation verified via reconnect/poll (not just WS delivery), focused Go/frontend suites + both lint gates rerun. Keep the row-provenance improvement; tighten or split out version-collapse and frontend-admission if they can't be proven. No merge until then. |
…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): 65/100 — needs work, not ready to merge.
The change is still needed after #220 (no same-hunk overlap; complementary).
Ordered fixes:
- Serving fallback bypass —
stream.go:859,879-901: carry the cross-row rejection throughGetByPath. The URI-only fallback must not reauthorize a rejected row. Test that fallback cannot bypass the check. - Cross-tier dedup —
playback_virtual.go:3977-3985withresolver.go:717-732: strongest-tier-only keys cannot tell a name-only candidate from a GUID/hash equivalent, so duplicates rotate instead of refusing. Compare compatible tiers or fail closed. Test mixed-tier duplicates without rotation. - Zero-ID sessions —
playback_service.go:1127: guard withsession.MediaFileID > 0, matchingstream.go:927. Add a push-path regression test. - Probed churn —
detail.go:4189-4207: applyneutralVirtualMediaPathin version grouping for probed entries too, keeping genuine version distinctions. Test two probed paths differing only inresult=. - Post-rotation refresh —
session.go:1472-1490: invalidate stale track evidence and trigger a probe of the replacement; keep the gate fail-closed until refreshed. Test rotation through recovery.
Hygiene: rebase onto current main (post-#220) 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.
Address five review findings on the exact-row evidence binding: - stream.go: carry the cross-row rejection through the URI-only live-row fallback in bindSessionVirtualSourceWithTracks, so a rejected sibling row cannot be reauthorized by GetByPath. - playback_virtual.go/resolver.go: compare compatible durable identity tiers symmetrically (PersistedIdentitiesMatch) instead of a strongest-tier key, so a name-only row and a GUID/hash row of one release are recognized as the duplicate they are and the owner rotation refuses. - playback_service.go: refuse the in-memory refused-probe override for a session with an unknown effective row id, matching the serving-path guard. - detail.go: group probed version entries on the shared neutral path so result= churn collapses, while a distinct size or conflicting identity stays a genuine version. - playback_service.go: after a committed rotation, re-resolve and re-probe the replacement through the bounded detached gate, generation-fenced so a superseded binding is never probed. The serve gate stays fail-closed until the replacement evidence lands. Regression tests cover each path.
|
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): 85/100 — needs work before merge. All five fixes verified present and tested, but two items remain:
- Lint failure on this head:
internal/api/handlers/playback_service.go:1037comment readsauthorise; themisspelllinter requiresauthorize. One-word fix. - Rotation race: in
probeRotatedVirtualCandidate, the generation check runs before resolution. Resolution and probing can outlive another rotation, andprobeVirtualSourceAndPersisttakes no generation, so the "superseded binding is never probed" claim is not established by the check alone. Add a deterministic test that rotates while resolution/probing is blocked and proves stale work cannot persist or authorize stale tracks — or point at the persistence invariant that already enforces it.
Also: Go test, changed-lines, and router-recovery gates still pending on the final head. Rebase onto current main if it has moved, then require green CI.
probeRotatedVirtualCandidate checked the binding generation only before resolution. Resolution and the probe are separate blocking calls that can each outlive a newer rotation, and probeVirtualSourceAndPersist took no generation, so superseded work could still mark the failure damper, release the sticky pin, or persist evidence for a candidate the session no longer serves. Thread a generation fence through the pipeline: re-check it after resolution and again inside probeVirtualSourceAndPersist after the probe returns and before any failure verdict, unpin, or catalog write. The variadic fence keeps the existing callers unchanged; only the rotated candidate refresh supplies one. Add a deterministic race test that parks rotation A in the resolve stage and separately in the probe stage, lands rotation B while A is blocked, releases it, and proves no evidence is persisted for the superseded candidate and the stale evidence cannot authorize serving.
|
Re-review follow-up pushed on
|
|
Both items from the latest review are now addressed and CI is green on the final head ( |
drondeseries
left a comment
drondeseries
left a comment
There was a problem hiding this comment.
Review: Request changes — one merge blocker remains (87/100)
Verified on 6cb9cf78, all 8 CI checks green. Row-bound evidence, PersistedIdentitiesMatch, misspell fix, and the post-probe fence are real progress. Not merging yet for one scheduling race below.
Blocker: rotation refresh pairs a stale file snapshot with a newer generation
internal/api/handlers/playback_service.go:959-978 (PublishSourceCommitted) does GetSession, then GetByID(session.MediaFileID), then refreshRotatedVirtualCandidateBackground(ctx, session, file), which at :1061 reads inventorySourceGeneration(session.ID) separately.
If a rotation A→B lands between the session/file snapshot and the generation read, probeRotatedVirtualCandidate (:1074-1140) re-reads the new live session (live, URI_B) and passes the fence (current == generation == new), but retains the old file (file_A) for isVirtualPlaybackFile, cloneVirtualProbeTransient, bestResultCacheKey(file.ContentID, ...live.URI...), and probeVirtualSourceAndPersist(..., file, ...). The post-probe fence does not close this scheduling window — only downstream persistence guards stand between the mismatched pair and the catalog.
Fix (minimal, ordered):
- In
PublishSourceCommitted, obtain the pair atomically via the existingsessionWithSourceGeneration(sessionID)(:1322, backed byGetSessionWithSourceGeneration), then load the file from that session'sMediaFileID. Pass that generation through torefreshRotatedVirtualCandidateBackgroundinstead of re-reading it separately. - In
refreshRotatedVirtualCandidateBackground, stop snapshotting generation apart from the session. Accept the atomic(session, generation)or re-derive both atomically inside, and reject if the binding moved beforego func(). - In
probeRotatedVirtualCandidate, after the live re-read + fence pass, reload/validate the file fromlive.MediaFileID(file.ID == live.MediaFileID, else drop) and use the live-derived file for the transient clone, neutral key, and persist call. - Add a deterministic channel-gated test that rotates between the file lookup and the generation capture and proves the stale pair is dropped (zero persisted paths, serve-gate rejection). Extend the existing
TestProbeRotatedVirtualCandidateRaceDoesNotPersistStaleEvidencerather than adding a sleep-based test.
Follow-ups (non-blocking, track separately)
- Fence is checked once before mutate/persist (
playback_virtual.go:3051-3089) — catches rotation during probe, not every rotation before persist completes. Don't claim atomic-commit semantics; rely on/document the persistence CAS. detail.godedup uses raw trimmed case-sensitive tier conflicts +ReleaseName, diverging from resolver-normalizedPersistedIdentitiesMatch; exact-unprobed key omits provider identity/declared size. Add differential cases (churned dupes retained vs. distinct unprobed rows collapsed).- Recovery is lossy by design: detached-gate exhaustion abandons re-probe, and
generation == 0disables fencing. Confirm rotated prod sessions always carry nonzero generations; document/test post-saturation recovery.
Merge once the blocker + its regression test land and the required checks are green. Happy to re-review the fix head.
PublishSourceCommitted read the session, then loaded the file from its MediaFileID, then let refreshRotatedVirtualCandidateBackground snapshot the generation separately. A rotation landing between the file load and the generation read left probeRotatedVirtualCandidate re-reading the NEW live session (so the fence passed) while still carrying the OLD file for the virtual checks, transient clone, cache key, and persist call. - PublishSourceCommitted now takes the (session, generation) pair from one read via sessionWithSourceGeneration (falling back to a session read plus a separate generation read), and loads the file from that session's effective row. - refreshRotatedVirtualCandidateBackground re-reads the pair before scheduling and drops the work when the binding moved or the file no longer names the live effective row. - probeRotatedVirtualCandidate re-derives the file from the live session's MediaFileID and drops the work unless it matches the carried file, so the transient clone, neutral key, and persist call all use the row the session actually serves. Extend the deterministic rotation-race test with a schedule window (the production scheduler, split session/generation reads) and an effective-row window, alongside the resolve and probe stages.
|
Blocker fixed and pushed (
|
drondeseries
left a comment
drondeseries
left a comment
There was a problem hiding this comment.
Re-review: blocker closed, approve (94/100)
f768f8ba + 7cb43f08 on 7cb43f08ab:
- Torn-pair blocker — closed.
PublishSourceCommittednow reads(session, generation)from one lock viasessionWithSourceGeneration;refreshRotatedVirtualCandidateBackgroundre-reads the pair before scheduling and drops when the binding moved orfileno longer names the live effective row;probeRotatedVirtualCandidatere-derives the file and refuses unlessliveFile.ID == live.MediaFileID == file.ID. TheMediaFileID <= 0path still fail-closes. The channel-gated race test covers the schedule/effective-row/resolve/probe stages. - Test compile break — fixed (generation read moved above handler construction).
CI: 8/8 green on 7cb43f08 (Go contract checks, Go lint ×3, Go test, Web ×3).
Remaining items are follow-ups, not merge blockers: dedup tier parity with resolver.PersistedIdentitiesMatch, lossy recovery after detached-gate exhaustion, generation == 0 fencing semantics, and persistence CAS vs fence claim.
LGTM — merging is unblocked from my side.
2 participants
Problem
Related issue: N/A
Validation tasks: none
Duplicate catalog rows for one release break the invariant the evidence system relies on: provenance is keyed on the virtual candidate URI, not the row. Probe evidence rotates onto sibling rows, and serve/resume paths apply any URI-matching evidence, so playing one candidate can serve another candidate's audio/subtitle tracks. Menus and served tracks then diverge between cold start and resume.
Approach
Record the evidence row id at plan time and require row id plus candidate URI to match when applying evidence or publishing refused-probe inventory (URI-only kept for legacy sessions). Refuse probe rotation onto duplicate sibling rows while keeping distinct-release rotation. Collapse byte-identical duplicate rows in the version list, keeping unprobed placeholders only when no probed copy exists.
Validation
Risks
Sessions bound to a duplicate row now only accept evidence recorded for that exact row. Legacy sessions without evidence row ids keep URI-only matching. No API, migration, or config impact.
Checklist
AI Disclosure