Skip to content

Code review - #54

Merged
iFlip721 merged 9 commits into
mainfrom
code-review
Aug 11, 2026
Merged

Code review#54
iFlip721 merged 9 commits into
mainfrom
code-review

Conversation

@iFlip721

Copy link
Copy Markdown
Contributor

Code review - identify bug and hot fixes - implemented on branch. Full unit testing completed.

Origin republish: egress correctness, ingest lifecycle, and telemetry fixes

Follow-up hardening pass over the stream-arch origin-republish work (#52). A review of the merged diff surfaced 28 distinct issues; this branch fixes 24 of them, with the remainder deliberately deferred and listed at the bottom.

No new features. No API or config changes. Behaviour on a healthy muxed channel is byte-identical.

Rust data plane — demuxed / raw-TS egress correctness

The origin's demuxed-ness can change under a live session, and each egress path sampled it at a different moment. That shared root cause produced four separate viewer-visible faults.

  • Stale demuxed state after a bare-TS re-resolve. resolve_media's bare-TS arm returned before the write that clears demuxed_audio, so a demuxed-HLS → raw-TS re-resolve left the dead rendition in place forever. The entry kept serving an authored master whose audio segments all 404'd, and raw-TS clients looped connect → decline → close, because each reconnect re-read the same stale flag.
  • Silent audio loss on a muxed raw-TS socket. ts_ring_producer never checked seg.audio, so after the ring turned demuxed mid-session it served the video lane only — permanently, with no decline cap and no recovery. It now ends the socket (close_reason = "lane_changed"), mirroring the recovery the pair producer already had for the opposite flip.
  • Unsignalled clock jumps around declined segments. The raw-TS decline arm served a segment verbatim without splicer.reset(), so the following normalized segment was spaced against a timeline the client never received — an output PTS jump in each direction.
  • Audio-lane playlists that 404 every segment. serve_playlist rendered the audio ladder unconditionally; it now 404s when a non-empty ring holds no audio lane.
  • Wrong implicit AES IV on the audio lane. RFC 8216 §5.2 derives an absent #EXT-X-KEY IV from the segment's own media sequence, but the audio fetch was passed the video lane's sequence — and PDT pairing exists precisely because the two lanes renumber independently. PairPick::Found now carries the partner's own absolute sequence. Three pairing tests extended to assert it.
  • Entry playlist kind changing mid-session is not repairable in-session; added note_entry_shape() so the transition is logged once, at warn, rather than presenting as an unexplained stall.

Rust data plane — ingest lifecycle and self-heal

  • Teardown now runs on every exit, including a panic. It was a tail of the ingest body, so a panic in a parser left a registry entry marked live for an ingest that no longer existed: the channel could never be respawned. Moved into an IngestGuard drop handler, which removes the entry only when the key still holds its origin.
  • Demuxed HLS sessions can now self-heal. Lane endpoints only read the registry, so once an idle teardown removed the entry every later poll 404'd forever — where a muxed client recovers transparently. Lane polls now subscribe and wait like the entry path, gated on a target this process has actually resolved so the routes stay side-effect-free for arbitrary input.
  • Structurally ineligible upstreams are remembered. A channel whose entry cannot be ringed re-ran resolve → spawn → fetch → decline → teardown on every manifest poll, per viewer. A 60s decline memo short-circuits that; the ring is dropped on the retain path so the memo holds the verdict, never the media.
  • Warn dedupe actually dedupes. The latch keyed on decline strings embedding live measurements, so every declined segment minted a unique key — one warn line per segment forever, and an unbounded HashSet in a long-lived task.
  • Ring generations are seeded rather than starting at 0, so two incarnations of a channel can never share one.
  • Provider-specific branch removed from the generic core. The undecodable-upstream detector was gated on a hardcoded source name — the only one in the crate. It now rides the resolve grant as playerSelectable. Absent field degrades to false, the safe direction.
  • Ring footprint metrics exclude stopping origins, so retained decline memos no longer inflate channel counts.

Rust data plane — muxer / normalizer

  • Raw-TS weave anchor. ANCHOR_BACKSTOP's premise was wrong: the skew guard bounds drift from the locked skew, not its magnitude, and PDT pairing accepts partners up to half a segment apart. A lane leading past the fixed 2s backstop sorted all of its units after all of the other lane's, emitting a backwards DTS step at every pair seam. The anchor is now derived from the earliest key actually present. Regression test verified to fail against the old anchor with a 2.7s backwards step.
  • Unreadable encryption is no longer reported as cleartext. An #EXT-X-KEY whose METHOD is unparsable mapped to NONE — the value reserved for measured cleartext. It now reports UNKNOWN; consumers gating on != "NONE" treat it as encrypted.
  • Single-pass audio-rendition classification. Two functions re-derived the same parsing rules over the same body; a fix to one and not the other would skew the demuxed-vs-muxed verdict — the exact bug that mis-serves an audio-description track as programme audio.

Control plane — telemetry, migration, maintainability

  • Version-skew states survive the seam. Absent iop fields from an older sidecar were materialized as authoritative values, making "not reported by this sidecar" branches unreachable: a declined channel showed a healthy ingest tile while the rewrite path served.
  • The adPolicy migration now runs. Mongoose strict-mode casting stripped the unknown path from the $unset, leaving an empty update that short-circuits before any DB call — so it never wrote, and modifiedCount was undefined, meaning its success log could never fire either.
  • Per-channel display maps are pruned by construction — six identical prune functions replaced with a registrar, so a new map cannot be added without being swept.

Frontend — two-clock skew in stream stats

Every timestamp in the stats payload is stamped by the server, but the screen aged them against the browser clock. A machine ten seconds fast read every session as stale, so the default-open OUTPUT stage reported "draining" indefinitely while viewers were streaming; a machine behind the server rendered negative ages such as "-42s ago".

One root-cause fix: the payload carries the server clock, the client derives an offset per frame, and all six affected sites became same-clock subtractions. Two were not in the original report: the per-client connected column, and ad-break age — which previously treated a break that opened before the screen did as brand new, so the stuck-break fault could only fire after the screen had been open long enough.

iFlip721 added 9 commits August 11, 2026 06:57
D2 is proven, not argued against a throwaway mongo:7 on an isolated port rather than trusting the source trace:
OLD  -> {"acknowledged":false} | adPolicy still on disk: true
        modifiedCount is undefined ; `modifiedCount > 0` evaluates to false
NEW  -> {"acknowledged":true,"modifiedCount":1,...} | adPolicy still on disk: false
NEW2 -> {"acknowledged":true,"modifiedCount":0,"matchedCount":0}   ← second boot, clean no-op
@iFlip721
iFlip721 merged commit db5f44e into main Aug 11, 2026
14 checks passed
@iFlip721
iFlip721 deleted the code-review branch September 1, 2026 11:29
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.

1 participant