Repository navigation
perf(playback): defer track inventory past first byte with live follow-up - #228
Conversation
|
Oracle review — request changes on three points:
The same-release retry does schedule the probe, and its test exercises the retry branch. Existing WebSocket |
|
Review addressed and pushed ( |
|
Oracle re-review of
The saturated-gate test establishes response-before-probe under saturation, and the codec test exercises the real resolve/merge. Neither covers these remaining cases. Tests inspected, not executed. |
|
Oracle production-readiness review of Overflow now terminates, and deferred success waits for the production database write. The generation CAS also closes rotation during persistence. However, lifecycle gaps and a new saturation-path delay remain.
Sign-off required: When the fix plan is complete and CI is green, request a final oracle review (tag @drondeseries) before merge. Merge is gated on that approval. |
|
@drondeseries requesting final oracle review as gated. Pushed |
…w-up A cold virtual start persists the full recipe/subtitle evidence before the first-byte URL is handed back, so session creation, recipe persistence, and the transport commit all wait on a multi-second remote ffprobe. The fresh-start resolve now defers its candidate-declared upgrade probe past the transport commit: the client gets its playable URL immediately, and the existing inventory_updated push (or the inventory poll named by inventory_url) delivers the probed track menu afterwards. The plan carries an additive tracks_pending marker so a client shows the menu as loading until the follow-up lands; it is excluded from plan identity hashing like the inventory fields beside it and never changes stream selection. The deferred probe uses the same detached gate and bounded context as the inline start-path probe, so only when ffprobe runs moves, never the concurrency budget or the persistence guarantee. A same-release transient transport retry that commits the deferred plan spawns the probe too, so a provisional plan always gets the follow-up it promises. Also carries the preprobe pin-liveness memo: a successful provider listing for a content is recorded for 45s, letting a subsequent cold start whose row already names a concrete candidate skip the full listing. A forced relist, an exclusion, or an unusable/failed row still lists, and a miss falls through, so recovery is never blocked. Frontend/client changes are out of scope; the feature is advertised on /api/v2 only as deferred_track_inventory_v1.
…tadata preservation
Fixes the nilerr finding on the retargeted main diff: the test push helper checked json.Unmarshal's error but returned nil, which golangci-lint flags on the changed-lines job.
…f-path Capture the candidate binding identity and generation together when a deferred probe is admitted and retain that generation for the probe's whole lifecycle: the pending write, the shed, the in-probe evidence fence, and the terminal outcome CAS all use it, and a worker never recaptures it. A probe is dropped when the session is gone or bound to a different candidate, and no outcome is ever written unconditionally. Move overflow publication off the start path: the failed session outcome is still stored immediately, but the notification is delivered by a bounded, coalescing background pool so a slow catalog read or realtime fan-out cannot delay the response. Complete regression coverage in both deferred test files and remove the dead liveness memo and its wiring, consolidating the outcome writer onto the fenced setter.
36586fc to
5e4d66f
Compare
|
Dev update — dispatcher notification recovery with generation-scoped ack pushed ( What this adds beyond
Coverage: 21 deterministic dispatcher tests in @randrini — this is ready for your look. Per the gated plan, once CI is green on this head, request the final oracle review (tag @drondeseries) before merge. |
Production-readiness: NOT READY — do not mergeReviewed head Must-fix before merge1. Expose the promised v2 wire contract — 2. Gate deferral on v2 + client capability — 3. Complete the client handoff What was checked and cleared (no action needed)
CI noteGo test/lint still in progress at review time; re-verify green on the fixed head before re-requesting review. |
The deferred track-inventory lifecycle (tracks_pending, inventory_url, the pending -> verified/failed session outcome) was applied to every fresh virtual start, including the frozen /api/v1 bridge. That grew the v1 wire shape it must not, and promised a provisional menu to clients that never negotiated one. Scope deferral to a start that both arrives through /api/v2 and advertises deferred_track_inventory_v1, and scope the terminal failed notification to attempts whose stored negotiation includes the feature. Carry the additive tracks_pending and inventory_url on the v2 wire plan, projected from PlanV3, and document the pending/verified/failed semantics plus the client handoff for Apple and Android. Tests: v1 unchanged, v2 without the feature unchanged, v2 negotiated deferred start, idempotent attempt replay keeping the marker, terminal failure ending loading without switching the viewer's audio track, and the failed-push scoping guard. The existing deferred lifecycle tests now negotiate on v2. Generated OpenAPI, web types and the contract-digest fixture follow the additive v2 fields.
|
API-wiring review addressed and pushed ( |
Production-readiness: NOT READY — do not merge (re-review of 1c4020e)Good progress since 0edd798: v2 wire fields + fresh-start gating are fixed. Two blockers remain. Must-fix1. Cross-surface replay hole — internal/api/handlers/playback_service.go:253-264 2. Client coordination still open — docs/playback-api.md:65-73 Advisory (not blocking)
CIstatusCheckRollup empty on this head — no green-gate evidence. Re-verify full CI green on the fixed head before re-request. |
requireAttemptAPISurfaceV3 only guarded the SubRip representation. The request digest excludes the API surface and the shared PlanV3 serializes tracks_pending, so the same start body could replay a deferred-v2 attempt through the frozen /api/v1 bridge, and a v2 deferred retry could adopt an attempt that was never negotiated to defer. Extend the guard to the deferred lifecycle on both axes: /api/v1 refuses an attempt that negotiated deferred_track_inventory_v1, and a v2 start/deferred negotiation refuses an attempt whose stored client and server features did not negotiate it. Replans pass nil and stay covered by the v1 refusal. A failed inventory push now fails closed when the attempt row cannot be read: without the record the negotiation cannot be established, so the terminal status is suppressed and left to the session's own poll instead of reaching a client that never negotiated the lifecycle. Document that inventory_url is not gated by the negotiation and that a replan never defers but still carries the marker. Tests: cross-surface start replay both directions, cross-surface replan, surface-guard matrix, concurrent-duplicate replay through the SaveAttempt collision branch, a real non-deferred v2 response carrying inventory_url, and the nil-record fail-closed push.
|
Review addressed and pushed ( |
Production-readiness: NOT READY — do not mergeReviewed head Must-fix1. High — terminal-save collision still replays deferred plans through v1. 2. High — failed notifications are acked as delivered. 3. Medium — duplicate starts re-arm a completed lifecycle. 4. Medium — #233 default-audio hook still bypassed. 5. Medium — Apple/Android handoff unfiled. Lows
Cleared since last round: saturated-gate off start path, generation+fencing end-to-end (A→B→A), durable-write ordering, overflow push+poll coverage, retry regression, v2 wiring + fresh-start gating. Merge gate: fix 1-5 with targeted regressions, refresh the PR body, request another readiness round. |
Close the four findings from the #228 review: - Guard the terminal-save collision replay with the API-surface check whenever the collided record carries a playable plan, so a losing duplicate cannot hand a deferred/SRT v2 plan back through v1. Terminal records still replay cross-surface on purpose. - Classify one session-targeted inventory publish as delivered, obsolete, or retryable. A retryable attempt (transient plan-store read, catalog build failure, or realtime write that did not reach the client) re-parks with a bounded backoff instead of acking an undelivered terminal status, so a push-only client is not left loading forever. - Arm the deferred-probe lifecycle pending only from empty (ArmVirtualProbePendingIfGeneration), so a duplicate start that adopted a winner's session cannot re-probe and re-arm a lifecycle the winner already armed or completed. - Run the shared reconcileVerifiedDefaultAudio hook after the deferred durable evidence write commits, against the owner row the write actually targeted, so the executable audio recipe keeps the preferred language. Tests cover both surface directions, cross-surface terminal replay, the one-shot store-failure retry with no reconnect or poll, collision after a winner's terminal outcome, and deferred default-audio reconciliation with explicit-selection preservation.
|
Review addressed and pushed ( |
Production-readiness re-review of
|
Address the second review round on the deferred track-inventory publish machinery. - Lazily initialize the pending and in-flight maps in the publish worker's cleanup, under deferredPublishMu, and always start the dispatcher before waking it. A retryable attempt on the unsaturated direct path never parks, so it previously wrote a nil map (a background-goroutine panic that would take the process down) and re-parked with no dispatcher to deliver it. - Stop the dispatcher spinning: drainDeferredPublishBatch now returns the sleep decision whenever nothing is dispatchable, instead of reporting an owed rescan as more work. A retryable entry waiting out its backoff keeps the map occupied and the inner loop would otherwise repeat with nothing to dispatch. - Let overflow recovery progress while a retryable entry waits: the rescan runs whenever the map has room, not only when it is empty, so a parked backoff entry cannot deadlock the overflow sessions behind it. - Route retry re-parks through reparkDeferredPublishRetryLocked, which enforces the pending-map bound; a retry insert at the cap retains the intent through the rescan flag instead of growing the map without limit. Tests: a direct-path one-shot publication failure delivers with no reconnect or poll (fails without the fix: nil-map panic, then a stranded retry); an overflow plus delayed-retry regression proves neither spin nor stall; a retry-insert bound test; and the lifecycle settle assertion waits on observable state instead of sleeping a fixed number of backstop ticks. Corrects the inventory-revision comment and the apiv2 wire-contract test comments to state what they actually assert.
|
Review addressed and pushed ( |
Production-readiness re-review of
|
The publish dispatcher selected a parked notification and deleted it from the pending map before claiming a gate slot. When the claim lost the race for a saturated gate, the batch unconditionally reinserted the intent, bypassing virtualDeferredPublishPendingCap: the map at its cap could have a slot freed by selection, a concurrent park could refill it, and the reinsert then grew the map to cap+1. Route the failed-acquisition repark through reparkDeferredPublishSelectedLocked, which applies the same coalescing rule and pending bound as every other park path and retains the overflow intent through deferredPublishRescan instead of inserting past the cap. The reinsert preserves the intent's schedule: a saturated gate is backpressure, not a failed publish, so the attempt count and not-before backoff are left untouched. Correct the parked/in-flight comments that still described the old delete-before-acquire ordering as if the entry stayed parked throughout. It does not: selection deletes the entry before the gate claim, and the claim's re-validation plus the failed-acquisition repark close the brief gap. Tests: a deterministic regression that selects the entry, refills the freed slot to the cap, drives a failed slot claim, and asserts the repark does not exceed the bound; and a companion asserting the repark does not count as a retry attempt or set a backoff.
|
Review addressed and pushed ( |
Final re-review of
|
2 participants
Problem
Closes #226
Base: main.
Cold virtual starts (~10.3s, all inside
session_transport_commit) synchronously persist full recipe/subtitle evidence before first byte; warm starts pay 80–139ms synchronous ffprobe.Approach
Persist a minimal recipe (video + default audio) to unblock transport commit; full audio/subtitle inventory moves to an async probe worker spawned after commit, emitting the existing
inventory_updatedpush with row-bound evidence. Additivetracks_pendingmarker +deferred_track_inventory_v1feature flag; client renders loading state until the push lands. A per-candidate provider-listing outage is classified and retried in bounded steps; there is no content-keyed liveness memo. Includes the same-release transient-retry path so the follow-up is never dropped. Seek, verdict, and anchor semantics untouched.Validation
go test -runsubsets incl.-racegreen (full-package runs skipped under server OOM budget); gofmt/vet/diff-check cleanRisks
Clients that block on the full track list see a loading state until the push (≤1s typical). Client coordination is recorded in PR discussion (not filed — maintainer exception requested: no external filing available from this lane; Apple/Android task lists + jellycompat exclusion in comments). No API removals, no schema, no new endpoints.
Checklist
AI Disclosure