Repository navigation
fix(playback): deliver the default-audio correction across reconnects - #233
Merged
Merged
Conversation
…eorder Persist the start-time audio selection intent (origin, preferred language, series snapshot, committed signature) on the attempt and the session, then replay it against the verified inventory when probe evidence lands. An automatic track_change replan moves the executable selection only when the language moved; identical selections stay a byte-equal no-op.
…orged provenance Establish the reconcile budget once at the entry point and propagate it instead of letting each interior caller discard cancellation and arm a fresh timeout. Clear client-supplied Automatic on the inbound replan boundary so only server-built reconciliation can claim that provenance.
…ption Reconciliation no longer self-commits a replacement plan the client never adopts. It persists the corrected decision atomically with the canonical request, withdraws the active plan with plan_invalidated, and lets the client's own replan commit the corrected recipe.
…keep the winner The pending default-audio correction was layered onto the replan request after the selection had already been copied onto the executable start, so audio resolution never saw it. Apply the correction first. A later divergence refusal is an audit record, not a verdict on the settled decision, so it must not strand a correction already announced.
…ntics The cold-start audio correction withdraws a plan whose route is healthy — the committed recipe plays the wrong audio stream, not wrong bytes — but the client treated every `plan_invalidated` as a `failure_recovery`, which folds the current plan's attempt key into `attempted_plan_keys`. That excluded the route that was playing from its own replacement plan, pushing a working session onto a worse route or a terminal. Replan off `default_audio_reconciliation` as `track_change` instead: it carries the audio correction, and since nothing failed the route stays eligible. Position and pause state are preserved as before, and `fallback_reason` still reports the server's own reason, so telemetry is unchanged and already distinguishes the two cases. Every other reason keeps the recovery semantics §6.1 specifies. The browser cannot import the Go constant, so each side pins the other's string with a test. No new operation or feature: the path reuses `track_change`, so the client capability list and the v3 operation enum are unchanged.
An explicit audio identity on the answering replan supersedes the pending automatic correction. A correction the server applies restores its own Automatic provenance, which the inbound boundary strips from clients, so a server decision is never persisted as a viewer preference.
…ponse The web request builder echoes the plan's own audio on every replan, so the answering request carries the withdrawn selection even when the viewer chose nothing. Presence is not intent: only an identity the withdrawn plan did not select is a viewer choice, and everything else still receives the correction, marked automatic.
The server now gates the default-audio reconciliation withdrawal on a new client capability, default_audio_reconcile_response_v1, and correlates the client's answer with its pending decision via an answers_plan_invalidation field on the replan. An older client that lacks the token gets no withdrawal and plays the corrected audio on its next start, instead of laundering the withdrawal through failure_recovery.
A client that would replay the withdrawal as failure_recovery excludes a healthy route from its own replacement, so the withdrawal is gated on a capability that means the client answers it correctly; older clients deliberately receive none and get the correction on their next start. The answering replan now correlates through answers_plan_invalidation instead of an inferred request shape, with the identity heuristic kept as the fallback.
…lity The capability golden records are the published list of what the server advertises, so the new feature token has to appear in them. The negative native fixtures are derived from the valid one and must differ from it in exactly their intended violation, so they carry the token too.
A capability-aware client is distinguished by its answers_plan_invalidation marker. Absent that marker on such a client, the request keeps its own selection rather than being treated as a reconciliation response, so a deliberate same-identity pick can no longer be silently replaced.
A successful hub send proved only that the server wrote the command. A client that disconnected before processing it, failed its replan, or reconnected later lost the correction for the rest of the session while the ledger claimed delivery. Delivery now stops when the attempt's current plan moves off the withdrawn plan, which any replan commits, and retries under a stable per-(session, generation) command id until then.
The web client sends answers_plan_invalidation when it answers a default_audio_reconciliation withdrawal, but the v2 replan schema did not declare the property. Because the v2 body forbids additional properties, a capable client's replan was refused outright rather than merely losing the correlation, so the correction could not land on the native surface at all. Add the optional request property to PlaybackReplanBody, map it in domain(), and regenerate the OpenAPI artifact, web types, and contract fixtures. The contract linter classifies the change as a new optional request property: additive and non-breaking, which is what the v2 additive-only rule requires before it locks. Also correct two protocol-doc errors found alongside it: the identity heuristic is a fallback for clients WITHOUT the echo capability, not for every mismatching echo, and the plan_invalidated example carried an HTML comment inside its JSON block. Add the missing conformance scenarios for the marked answer and the unmarked re-pick, so the correlation is pinned on the wire instead of only in prose. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A newly saved attempt carries an empty audio_reconcile_ledger '{}', and
'{}'::jsonb->>'revision' is SQL NULL. Both ledger reads cast that expression
to bigint and scan it into an int64, so the first read failed and the durable
reconciliation ledger could never bootstrap on the production Postgres store:
no correction could be recorded or announced for an ordinary new attempt.
Wrap both reads in COALESCE(..., 0). Changing only the JSON tag or the migration
default would not cover rows already written as '{}'.
Add a Postgres-backed regression that saves an ordinary attempt, reads revision
zero, records the first decision, and reads revision one. It is gated on
SILO_TEST_DATABASE_URL like the rest of the planstore suite, so it skips where
no migrated database is configured and runs in CI.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The repeated "mul" and "unknown" literals in audio_select.go tripped goconst and turned both lint CI jobs red. Extract the sentinel strings into named constants used at both sites, following the typed-const convention of the surrounding playback package. No behavior change; the playback suite still passes and lint-changed reports 0 issues. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
Oracle production-readiness review of
Also, reconnect marks expire after 10 minutes while cooldown lasts 15, so an exhausted session reconnecting between those bounds does not rearm. The migration and NULL-safe ledger reads look correct. Both columns are actually I reviewed source at NEEDS_FIX |
Collaborator
Author
|
@randrini — playback diagnosis since the last Vio start (build 932, rev One caveat: the provider log ( Aggregates
Per play Click-play is
Issues
Planning holds at ~0 ms with Sources: Harness: OpenCode. Model: workhorse (omniroute). |
This was referenced Oct 6, 2026
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.
Problem
A movie whose probe reorders the audio tracks starts in the wrong language. The server detects the stale default, settles one correction, and withdraws the plan — but if the client disconnects, fails the replan, or reconnects cold, the withdrawal was already marked delivered and never repeated. The correction was lost for the rest of the session and the wrong language played on until restart.
Two further defects surfaced during review. The v2 replan schema did not declare
answers_plan_invalidation, and because that body forbids additional properties a capable client's replan was refused outright rather than losing the correlation — so the feature worked on v1 and was broken on v2. And both Postgres ledger reads castaudio_reconcile_ledger->>'revision'to bigint; a fresh attempt's ledger is{}, which yields SQL NULL, so the first read of any new attempt failed and the durable ledger could never bootstrap on the production store.Related issue: N/A
Validation tasks: none
Approach
Delivery now stops on completion, not on send: the withdrawal repeats until the attempt's plan id moves off the withdrawn plan, bounded by a burst of 20 accepted deliveries, a 15-minute cooldown, and budget rearmed on a genuine reconnect (the reconnect rearms budget; subsequent evaluations deliver again). The budget is process-local, not cluster-global. Missing sockets do not consume budget. Capacity is reserved atomically before each send under one lock, and failed writes refund against an epoch so a stale failure cannot weaken a rearmed burst.
The correction is gated on a new client capability,
default_audio_reconcile_response_v1. A client that advertises it answers a withdrawal by naming its reason on the replan; the server treats a missing marker from such a client as an ordinary re-pick and keeps the viewer's selection. The identity heuristic remains the fallback for clients that have not adopted the echo, and for them alone.The v2 replan body declares the answer as an optional request property. The contract linter classifies it as a new optional request property — additive and non-breaking, as the v2 additive-only rule requires before it locks.
Validation
cannot scan NULL into *int64, so the test is proven non-vacuous.Risks
Adds one migration (two nullable jsonb columns defaulting to
{}) and one client capability. The capability is opt-in; clients that do not advertise it keep the previous behavior, with the correction deferred to the next cold start. Server-owned provenance is still stripped at the ingress boundary, so clients cannot claim server-owned reconciliation provenance. Apple and Android clients do not yet send the answer, so they continue on the deferred path until they adopt the capability.Checklist
AI Disclosure