Skip to content

fix(diagnostics): preserve source failures and request ownership - #818

Open
yucelokan wants to merge 2 commits into
superuser404notfound:mainfrom
yucelokan:codex/source-failure-diagnostics
Open

yucelokan wants to merge 2 commits into
superuser404notfound:mainfrom
yucelokan:codex/source-failure-diagnostics

Conversation

@yucelokan

@yucelokan yucelokan commented Oct 17, 2026 •

Copy link
Copy Markdown
Contributor

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

  • Preserve historical HLS/Demuxer NSError domains and case codes; expose finite HLS reasons and the original FFmpeg code separately.
  • Keep failed live starts classified as liveSourceUnavailable, with the original URL domain/code. HTTP refusals and TLS trust failures retain their specific classifications.
  • Capture diagnostic ownership before detached opens and pass it through AVIO probes, reconnects, prefetch, chunk and streaming delegates. Session reopens retain the same identity; explicit nil stays unattributed.
  • Make diagnostic events immutable and use extensible, string-backed Role and Phase types without changing their raw tokens. Hosts still own URL redaction.
  • Check HLS reason literals against the finite mapping and document that historical playlistUnreachable(status:) also covers segment/key HTTP refusals.
  • Make the streaming read-loss regression cut its connection only after a successful open/read, so an initial URL error cannot race the mid-read EIO assertion. The blocking work runs on a cancellable test worker.
  • Rebase on current main (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

  • Device / OS: MacBookPro18,1; macOS 27.0.1 (26A434); Xcode 27.1 (27A9275).
  • Source media: In-process HTTP range fixture with deterministic bytes for AVIO open/seek/reconnect; synthetic URL/HTTP/TLS failures and malformed HLS parser inputs. These validate transport and diagnostic contracts, not audio/video decode.
  • Result: At 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.
  • New regressions cover attributed and unbound reconnects across a real thread boundary, session reopen identity, live error compatibility, future diagnostic tokens and source-literal mapping drift. Existing error-code, reload-identity and public API/documentation checks are included.
  • The CI-only streaming-drop race was reproduced by the failure ordering and repaired in 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 at 7d643f26 passed 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.
  • No new physical-device playback or local full-suite run for this revision. Earlier validation described different revisions; the results above apply to this update.

Checklist

  • CHANGELOG.md updated
  • Commit messages follow Conventional Commits
  • The fix lives in the engine, not in a host-side workaround
  • Public API changes are intentional and documented

@superuser404notfound superuser404notfound left a comment •

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Posted twice by accident, the review below is the same text.)

@superuser404notfound superuser404notfound left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 / .sourceRateLimited with 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 in underlyingDomain / 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.

@yucelokan
yucelokan force-pushed the codex/source-failure-diagnostics branch from b9169f3 to 6f1608d Compare October 21, 2026 20:54
@yucelokan
yucelokan force-pushed the codex/source-failure-diagnostics branch from ad4b342 to 7d643f2 Compare October 21, 2026 21:27
@yucelokan

Copy link
Copy Markdown
Contributor Author

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants