Repository navigation
Tell a viewer and a data team the same thing about a failure nothing rescued - #197
Merged
Merged
Conversation
…rescued Rung 6 of ADR-0011's ladder, the top, and the half of "zero unclassified errors" that faces outwards. It closes #86 as rules 3 and 4 said it would. A failure every rung below has declined now ends the session on `SuperPlayerError`: `PRD.md` §3.2's shape — a cause class, a `userMessageKey` and `isRetryable` — plus rule 10's three, the rungs tried, the position reached and the likely party a `StaleLivePlaylistException` named. It arrives where errors already arrive, as the `cause` of the `PlaybackException` `onPlayerError` carries, with no new listener and no callback. The engine cannot carry it — a cause is fixed when an exception is built — so the delivered exception is core's, with the engine's own code and message and the engine's exception underneath, and `player.playerError` hands back the same object so a consumer reading the property is not told a different story. The other half is telemetry, and it reads the classifier rather than keeping one: `PlaybackFailure.classification` is the class's stable name, asked of the player through `SuperPlayer.classify` because a collector watches Media3's analytics rather than the facade and `superplayer-telemetry` is not a friend of core. `category` stays six values and is derived from the class where there is one; `code` stays `errorCodeName`, because which code the engine raised and what SuperPlayer made of it are two facts and a pipeline needs the first to find the failure in a logcat. That derivation is what moves `TelemetryEvent.SCHEMA_VERSION` to 2. The class and the band disagree for five kinds of failure — a playlist frozen at the origin, a read past the end of a segment, an unsupported format, the renderer bands, and every code the band did not recognise — and `docs/telemetry-schema.md`'s release note is the table of them. An added field would not have moved it; a changed bucket does. The rungs tried are a per-player `ClimbRecord` written where a rung takes a failure on and cleared when core says the content changed, so a new programme is not reported with the last one's history. A player without resilience classifies nothing, reports null, buckets off the band and receives Media3's own error unchanged, which is counted rather than assumed; the golden traces did not move. Closes #183 Closes #86 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HeTWwmK1KcFMrfSAre23GS
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
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.
Rung 6 of ADR-0011's ladder, the top: a failure every rung below has declined ends the session on a typed, actionable error, and telemetry reports the classifier's answer instead of an unexplained I/O error.
Closes #183
Closes #86
What is built
Half one — the error.
SuperPlayerError(core, public, internal constructor) isPRD.md§3.2's shape — a cause class, auserMessageKey,isRetryable— plus ADR-0011 rule 10's three: the rungs tried, the position reached, and the likely party where core's own detection named one (StaleLivePlaylistException.likelyCausesurvives the mapping). It arrives asPlayer.Listener.onPlayerError'sPlaybackException.cause. No new listener, no new callback.Half two — telemetry (#86).
PlaybackFailuregains a nullableclassification, the class's stable name.QoeCollectorasks the player (SuperPlayer.classify(error)) and keeps no taxonomy of its own, in either direction.categorystays six values, derived from the class where there is one and from the error-code band where there is not.LogcatSinkandSessionTraceRecorderappend the classification only where there is one.Argued decisions
The version bump: yes, and the issue's reason was not quite the reason. ADR-0011 rule 3 owes a bump only where the class and the band-derived bucket disagree and the bucket changes to match. They do, in five places, and the table is in
docs/telemetry-schema.md's new Release notes section:NETWORKSOURCE(Content.SegmentGap)NETWORKSOURCEERROR_CODE_DECODING_FORMAT_UNSUPPORTEDDECODERSOURCE(Fatal.Unsupported)RENDERERDECODERRENDERERgoes empty with resilience attachedUNKNOWNNETWORKSo
SCHEMA_VERSIONis 2. Worth recording: ADR-0011's own Consequences predicted this would first bite onStaleLivePlaylistException, and that turned out half right — theINTERMEDIARY_CACHEvariant isTransient.CdnEdge, whose row isNETWORK, the same as the band. It is theORIGINvariant that disagrees. Noted in an addendum at rule 3.codekeeps its meaning, against #183's own acceptance criterion. The issue expects the classification to arrive asPlaybackFailure.code. ADR-0011 rule 3 is narrower and saysclassification; the ADR wins, and the argument is the rule's own: which code the engine raised and what SuperPlayer made of it are two facts, a pipeline needs the first to find the failure in a logcat, and collapsing them deletes the engine's answer rather than adding SuperPlayer's. Recorded in the rule 3 addendum and in the schema doc. The acceptance criterion is met in substance — a classified failure is identified as such through the harness — but onclassification, not oncode.Telemetry reads through the facade rather than becoming a sixth friend of core. A collector watches Media3's analytics, so the exception it is handed is the engine's with nothing of this on it.
SuperPlayer.classifyis public for the reasonexoPlayeris public:superplayer-telemetryreaches core as a consumer does (ADR-0008), and the alternative — a sixth Kotlin friend — is a bigger architectural claim than one query method for a module that deliberately uses the public escape hatch.The delivered exception is core's, not the engine's. A cause is fixed at construction and the engine constructs the one it raises, so the delivery is a
PlaybackExceptionwith the engine's code and message,SuperPlayerErroras its cause, and the engine's exception under that — nothing lost, a consumer switching onerrorCodeunaffected.getPlayerError()returns the same object, which narrows rule 10's addendum (property not special-cased for withholding) rather than contradicting it: that addendum's reason — a consumer reading the property sees what their listener was told — is why it must be substituted.The classification is taken for every surfaced failure, eagerly. Not only at rung 6: rule 3 has telemetry report the class of a repaired failure too, and the position on the answer must be read before a rung moves it.
The seam grew two members.
PlayerStateRungs.typedErrorFor(error, positionMs)andforgetClimb()— the latter the one member that is not a question, because the rungs tried are the ladder's record and when they stop being this content's is core's fact. Neither needs a slot core calls, so rule 13's addendum's test is answered.PlayerStateLadderis now per player (aClimbRecordis a player's own history; a shared one would report one feed row's retries on another's error).Tests
TypedErrorPlaybackTest(new,superplayer-resilience, throughPlaybackHarness, nothing past the facade):RETRY_SAME_URLamong the rungs tried;player.playerErrorare handed one object, with the engine's code preserved;no-cacheisTransient.CdnEdgewithlikelyCause = INTERMEDIARY_CACHE, and itsPlaybackFailurecarriesclassification = "Transient.CdnEdge",category = NETWORK,codestillerrorCodeName— Telemetry reports a typed failure as an unclassified I/O error #86's acceptance;classifyanswers null, and itsPlaybackFailureis byte for byte what it was before Phase 5.SuperPlayerDecoderRecreationTestnow asserts the whole question order core puts about one failure —forgetClimb,typedErrorFor,opensNextSource,recreatesDecoder— which pins "the classification is taken before any rung moves the position".superplayer-resiliencegains a test-only dependency onsuperplayer-telemetry(phase 5 on phase 2), the same direction and reason as the existingsuperplayer-cacheone.Golden traces
./gradlew updateGoldenTraces, run alone: no diff. The four traces are core-only sessions with no failure line in them, and the trace's classification field is appended only where there is one — so ADR-0011 rule 14's "a core-only golden trace that changes is a violation whatever the diff says" is satisfied by there being nothing to review.Two-axis self-review
Standards. Clean-room citations kept (
// ref:on the RFC statuses,// spec:unchanged); ADR-0011 rules 1–3, 10, 13, 14 followed and the two shape decisions recorded as addenda rather than as undocumented exceptions;docs/api-surface.mdhonoured —updateApiSurfacerun, diff committed (PlaybackFailuregains a component,SuperPlayer.classifyandSuperPlayerErrorappear,FailureClassgainsuserMessageKeyand its key constants);docs/testing.mdhonoured — the test drives the public API only, and an early draft that readplayer.exoPlayer.playerErrorfor the engine's code was rewritten to read it off the facade. Smells considered: the two lambdas onreportingSourceAsare a mild data clump, kept because both concern the same callback pair and are documented together;classifymemoizes and so is a query with a side effect, documented wherewithholdsFromConsumeralready is; no secondwhenover an error code was added anywhere.Spec. Every acceptance box of #183 and #86 is met except the one on
code, which is deliberately not met and argued above.PlaybackFailure.classificationis null without resilience; the coarse category is unextended; the schema doc states whatcodemeans, how it relates toerrorCodeNameand whatclassificationis, and carries the release note.Not run, and why
Emulator and demo runs skipped (host memory, per the batch instructions).
benchmark/is a separate build and not incheck; it is unaffected by construction —PlaybackFailure's new field is defaulted, andStockTelemetryAgreementTestattaches itsQoeCollectorto a SuperPlayer built without resilience, so both collectors still derive identical events.CI
Dispatched on
issue-183-typed-error(CI isworkflow_dispatch:only; nothing runs on a pull request):https://github.com/ramesh130/superplayer/actions/runs/35066543083 — success.
🤖 Generated with Claude Code
https://claude.ai/code/session_01HeTWwmK1KcFMrfSAre23GS