Repository navigation
Conversation
superuser404notfound
left a comment
There was a problem hiding this comment.
Thanks for this, the cause-preservation work is a real improvement: keeping the historical NSError domains and case codes while carrying the FFmpeg code separately, terminalError staying source compatible, zero cost while no handler is installed, and the retired ranged connection no longer poisoning its forward-only replacement are all things I want in the engine. A few points before it can go in, roughly in order of weight.
1. Rebase onto main. The branch conflicts in CHANGELOG.md, AetherEngine+ReloadWithOptions.swift and LiveStreamRefusalRetryTests.swift. The flaky give-up test was already fixed on main in #809 (2394ab2e: the refusal budget is passed per reader and the give-up case is bounded from below instead of counting requests), so b9169f38 should probably be dropped in favour of main's version rather than merged on top of it. If something in it is still needed after the rebase, a short note on what main's fix does not cover would help.
2. A failed live start should stay .liveSourceUnavailable. Before this PR, a live probe that burned its reconnect budget (AetherEngine.swift live fail-fast) and a transport failure in the live HLS ingest both published .liveSourceUnavailable. Now both go through publishLoadFailure, so a timeout or a refused connection arrives as .sourceOpenFailed. Hosts branch on that kind: Sodalite maps .liveSourceUnavailable to its localized "live channel unavailable" screen and would fall back to a generic engine-classified error for every dead channel. api.md treats the kinds as API, and a host cannot tell this move apart from a new failure class.
I'd suggest keeping the new detail without moving the kind:
- HTTP status:
.sourceRefused/.sourceRateLimitedwith the status (this matches what the live HLS path already did, fine as is). - TLS trust:
.sourceCertificateRejected(also fine). - Everything else on a live start:
.liveSourceUnavailable, with the original URL error inunderlyingDomain/underlyingCode.
The test "Live initial connection failure keeps the URL error instead of generic live unavailable" would then assert the kind plus the preserved domain and code.
3. The diagnostics context does not reach AVIOReader requests. The HLS readers store their context explicitly, but AVIOReader never receives one: every delegate (Probe, ChunkFetch, PersistentRead, Streaming, TailPrefetch) reads the ambient task-local when it is constructed. The size probes run on Thread.detachNewThread (around AVIOReader.swift:4438 / 4451), and reconnects, prefetch and chunk fetches run on the FFmpeg read thread or the prefetch threads, where the task-local is nil. So on the progressive path, which is the most common one for a Jellyfin direct play, most events arrive without a context. Capturing SourceNetworkDiagnostics.context once in AVIOReader.init (or passing options.sourceDiagnosticsContext where the reader is built) and handing it to each delegate would make it hold, ideally with a test that drives a reconnect and checks the context on the second request.
4. diagnosticReason matches on message text. Rewording any playlistInvalid(reason:) or segmentDecryptFailed(reason:) string silently drops it into the fallback bucket. A test that collects the reason: literals thrown in Sources/ and checks that each maps to a non-fallback value would catch that drift. Small thing, but the whole point of the field is that it stays stable.
5. Key fetch refusals. An HTTP failure on the AES key now throws playlistUnreachable(status:) instead of segmentDecryptFailed. Publishing it as .sourceRefused with the status seems right to me, but the public terminalError then calls a key refusal a playlist one. Either a word in the doc comment for playlistUnreachable or a note in the changelog would do.
6. Public Event shape. role and phase are free Strings, and several fields are public vars on a type hosts only ever read. String-backed structs like PlaybackErrorKind (stable raw values, room to add) and let fields would make this easier to keep stable. Raw URLs in the event are fine given the documented redaction requirement.
Happy to take another look once it is rebased. Points 2 and 3 are the ones that block; 4 to 6 could also land as follow-ups if you prefer.
b9169f3 to
6f1608d
Compare
ad4b342 to
7d643f2
Compare
|
Thanks for the detailed review! I've addressed all six points and rebased onto main, keeping the existing #809 fix. Failed live starts retain liveSourceUnavailable and the underlying cause, and request context now survives AVIO thread hops and reconnects. The diagnostic contracts, documentation, and regression coverage have also been updated. All four CI checks are now passing. Thanks again for catching these compatibility and attribution issues! |
Summary
Preserve source failure causes and keep request diagnostics attached to the load that created them. Retiring a failed ranged request also clears its stored failure so a healthy forward-only replacement can open.
What changed
liveSourceUnavailable, with the original URL domain/code. HTTP refusals and TLS trust failures retain their specific classifications.RoleandPhasetypes without changing their raw tokens. Hosts still own URL redaction.playlistUnreachable(status:)also covers segment/key HTTP refusals.7.45.0). Drop the separate refusal-test workaround in favor of main's fix(tests): LiveStreamRefusalRetryTests no longer flakes: per-reader budget and backoff #809 fix; dependency declarations and that test now match upstream.These changes remain together because the failure cause and request owner must survive the same open/replacement lifecycle. No retry budgets, TLS policy or host fallback policy are changed.
Test plan
6f1608d7, 62 scoped unit tests passed (35 XCTest + 27 Swift Testing), with all four production traits enabled. The package and ExampleSources compiled; documentation links/anchors passed.ad4b342d; its targeted unit test passed against the same reader/test sources in the 7.45.0 fork integration. An unchanged upstream tail-prefetch test hit its 4 s HTTP timeout under runner load on the next CI run. The identical tree at7d643f26passed all four checks on GitHub Actions: macOS default/all-traits tests and both single-thread runs, iOS/tvOS/visionOS builds, runtime regressions and documentation links. No timing assertion was weakened.Checklist
CHANGELOG.mdupdated